Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Use render function in Actions component #2911

Merged
merged 3 commits into from
Aug 9, 2022

Conversation

raimund-schluessler
Copy link
Contributor

@raimund-schluessler raimund-schluessler commented Aug 2, 2022

This PR aims at fixing #2902. It changes the Actions component to use a render function instead of a template and introduces the Button vue component as popover trigger and single action button. #2902 is fixed for me when checking e.g. on the Tasks dashboard. I hope that I have correctly re-implemented all features of the Actions component, but this needs extensive testing, especially since this component is so widely used throughout Nextcloud.

Since Actions now uses Button, there are some visual changes to the Actions trigger, which align the appearance to the Button component (and make the Cypress test fail for now). However, I don't know if this is wanted for the Actions component. I guess, that's a question for @jancborchardt and @nimishavijay. I don't have any before/after screenshots, because I think it's best viewed in the docs:
Before: https://nextcloud-vue-components.netlify.app/#/Components/Actions?id=actions-1
After: https://deploy-preview-2911--nextcloud-vue-components.netlify.app/#/Components/Actions?id=actions-1

The only issue known to me (yet) is, that the close-after-click prop of e.g. the ActionButton introduced in #622 does not work in the production build, because this.$parent of the ActionButton is null (for reasons unknown to me). With a dev build, like in the docs, it works just fine. Any hint here would be very welcome.
I just noticed that this is broken on master too when using npm link, close-after-click has no effect anymore. It works with 6.0.0-alpha.0 installed via npm though. So I guess this is either an issue with npm link or this is a "feature"/bug of vue 2.7.8 which we bumped in #2929. I will check and create a separate issue for this if necessary.

@raimund-schluessler raimund-schluessler added this to the 6.0.0 milestone Aug 2, 2022
@raimund-schluessler raimund-schluessler added bug Something isn't working regression Regression of a previous working feature labels Aug 2, 2022
@raimund-schluessler
Copy link
Contributor Author

This PR is ready for a first round of reviews.

@raimund-schluessler raimund-schluessler marked this pull request as ready for review August 6, 2022 20:40
@raimund-schluessler raimund-schluessler added 3. to review Waiting for reviews feature: actions Related to the actions components and removed 2. developing Work in progress labels Aug 6, 2022
@nickvergessen nickvergessen removed their request for review August 8, 2022 06:15
Copy link
Contributor

@marcoambrosini marcoambrosini left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Tested in the docs and work, code looks good to me too. I can't test in Talk because everything is still messed up by the floating vue stuff.
@raimund-schluessler can we create a separate pr with the cypress fix and get that in asap?

@raimund-schluessler
Copy link
Contributor Author

@raimund-schluessler can we create a separate pr with the cypress fix and get that in asap?

What cypress fix do you mean?

@marcoambrosini
Copy link
Contributor

Oh nvm, I thought you fixed the snapshots that were failing on master here

@raimund-schluessler
Copy link
Contributor Author

Oh nvm, I thought you fixed the snapshots that were failing on master here

No, I just had to update the snapshots, because we use Actions in the sidebar and it's appearance changed, because we use the Button component now for the popover trigger and single action button.

The snapshots look fine on master, though.

Copy link

@nimishavijay nimishavijay left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

However, I don't know if this is wanted for the Actions component.

Consistency for similar functionality is a good thing! It looks great now (especially the keyboard a11y 💯)

@raimund-schluessler
Copy link
Contributor Author

@marcoambrosini Can we do a 6.0.0-alpha.1 release after this is merged? Since it's a big change, I wouldn't want to go for a final release immediately.

Signed-off-by: Raimund Schlüßler <[email protected]>
Signed-off-by: Raimund Schlüßler <[email protected]>
Signed-off-by: Raimund Schlüßler <[email protected]>
@marcoambrosini
Copy link
Contributor

Can we do a 6.0.0-alpha.1 release after this is merged? Since it's a big change, I wouldn't want to go for a final release immediately.

@raimund-schluessler sounds good but don't we want this in v5.5.0 too?

@raimund-schluessler raimund-schluessler added the breaking PR that requires a new major version label Aug 8, 2022
@raimund-schluessler
Copy link
Contributor Author

@raimund-schluessler sounds good but don't we want this in v5.5.0 too?

I wouldn't backport this. It's a quite big change, and it relies / uses changes that are only present in master / 6.0.0 e.g. the migration to floating-vue (which is breaking).
Also, I didn't implement icon-classes for the actions trigger and single action, since Button does not really support it (you could put it in the slot, though) and I thought we anyway discourage the usage of icon-classes with 6.0.0.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
3. to review Waiting for reviews breaking PR that requires a new major version bug Something isn't working feature: actions Related to the actions components regression Regression of a previous working feature
Projects
None yet
5 participants