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

feat(NcActions): Allow to manually specify the semantic menu type #5336

Merged
merged 2 commits into from
Mar 11, 2024

Conversation

susnux
Copy link
Contributor

@susnux susnux commented Mar 1, 2024

☑️ Resolves

So we can make use of NcActions even when wrapping common groups of actions in components.

🏁 Checklist

  • ⛑️ Tests are included or are not applicable
  • 📘 Component documentation has been extended, updated or is not applicable
  • 3️⃣ Backport to next requested with a Vue 3 upgrade

@susnux susnux added enhancement New feature or request 3. to review Waiting for reviews feature: actions Related to the actions components labels Mar 1, 2024
@susnux susnux added this to the 8.9.0 milestone Mar 1, 2024
@juliusknorr juliusknorr modified the milestones: 8.9.0, 8.10.0 Mar 6, 2024
Copy link
Contributor

@ShGKme ShGKme left a comment

Choose a reason for hiding this comment

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

Works, and I also thought about this way, but I'm not sure it is a good way to solve the issue.

  • It fixes only 1/3 issues with non-direct children.
    • we can make use of NcActions even when wrapping common groups of actions in components only applies to a11y issues, we still cannot use it having inline and single button features.
  • It must be used carefully because it can result in completely incorrect behavior, for example:

At the same time, I don't see any simple full working solution with current implementation.

The only fully working alternative is to always "render" all the slot content, but make all the NcAction* "renderless" and use them as a config transfer object like NcAppSidebarTab does. It should require quite a huge implementation change and always have 2-stage rendering.

So I'm fine with it as a simple temporary solution if there are cases where it is required.

As far as I know, in server, photos, text, activity there are no issues with this limitation. But there could be in mail, talk.

src/components/NcActions/NcActions.vue Outdated Show resolved Hide resolved
src/components/NcActions/NcActions.vue Outdated Show resolved Hide resolved
Copy link
Contributor

@emoral435 emoral435 left a comment

Choose a reason for hiding this comment

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

Really nice documentation for a11y!

src/components/NcActions/NcActions.vue Outdated Show resolved Hide resolved
Signed-off-by: Ferdinand Thiessen <opensource@fthiessen.de>
@susnux susnux force-pushed the fix/actions-discover-children branch from 9ca3647 to a7c4f08 Compare March 11, 2024 11:33
@susnux susnux added 4. to release Ready to be released and/or waiting for tests to finish and removed 3. to review Waiting for reviews labels Mar 11, 2024
Signed-off-by: Ferdinand Thiessen <opensource@fthiessen.de>
@susnux susnux force-pushed the fix/actions-discover-children branch from a7c4f08 to 8c72eb4 Compare March 11, 2024 12:02
@susnux susnux merged commit ba95036 into master Mar 11, 2024
9 of 10 checks passed
@susnux susnux deleted the fix/actions-discover-children branch March 11, 2024 12:03
@susnux
Copy link
Contributor Author

susnux commented Mar 11, 2024

/backport to next

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
4. to release Ready to be released and/or waiting for tests to finish enhancement New feature or request feature: actions Related to the actions components
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants