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

fix(NcActions): hotfix for custom children #5178

Merged
merged 2 commits into from
Jan 30, 2024

Conversation

ShGKme
Copy link
Contributor

@ShGKme ShGKme commented Jan 30, 2024

☑️ Resolves

In NcActions we check what NcAction* components were passed as direct children. But some apps (Text, Mail) pass NcAction* not as direct children of NcActions, but as a sub-child.

<NcActions>
  <MyUserActionsList /> // renders <NcActionButton />
</NcActions>

The current NcActions implementation doesn't fully support this.

  • In past it resulted in ignoring inline prop and incorrect render of single actions
  • After mentioned PR it was completely broken:
    1. Menu is rendered
    2. There are no direct NcAction* interactive children (they are sub-children)
    3. Menu is considered a tooltip
    4. It should close on blur
    5. After opening, it focuses on an interactive element, causing blur and closing.

Unfortunately, in Vue 2 it is not possible to easily fix. When render function is executed, children are not rendered yet, so we don't know if they are to render NcAction* components or not.

This PR adds a hotfix that prevents component from being unusable. If children are not allowed custom components - consider menu a dialog with the default focus trap.

So the component works like it worked in the past. Yet it still has bad accessibility in this case.

🖼️ Screenshots

Docs

image

🏚️ Before 🏡 After
nc-actions-children-before nc-actions-children-after

🚧 Tasks

  • Document NCActions children limitations
  • Add a hotfix - make NcActions be considered a semantic dialog if there are custom

🏁 Checklist

  • ⛑️ Tests are included or are not applicable
  • 📘 Component documentation has been extended, updated or is not applicable

@ShGKme ShGKme added bug Something isn't working 3. to review Waiting for reviews feature: actions Related to the actions components regression Regression of a previous working feature labels Jan 30, 2024
@ShGKme ShGKme added this to the 8.6.1 milestone Jan 30, 2024
@ShGKme ShGKme self-assigned this Jan 30, 2024
Copy link
Contributor

@raimund-schluessler raimund-schluessler left a comment

Choose a reason for hiding this comment

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

These custom wrappers for NcAction* are a pain. I know they are necessary sometimes, but they really interfere with how NcActions works. And it got worse with the focus trap and a11y stuff.

However, the hotfix brings back the functionality for now, so 👍

I just had a few nitpicks for the language 🙂

Copy link
Contributor

@Antreesy Antreesy left a comment

Choose a reason for hiding this comment

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

Tested with styleguide only

@mejo- mejo- force-pushed the fix/nc-actions--custom-children branch from d58b498 to aa8b796 Compare January 30, 2024 10:25
Copy link
Contributor

@susnux susnux left a comment

Choose a reason for hiding this comment

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

Please fix DCO and the fixup commments

@ShGKme
Copy link
Contributor Author

ShGKme commented Jan 30, 2024

Please fix DCO and the fixup commments

fixup commits are not supposed to be merged without squash

@ShGKme ShGKme force-pushed the fix/nc-actions--custom-children branch 3 times, most recently from 08cbc01 to 22f02b7 Compare January 30, 2024 11:02
@susnux
Copy link
Contributor

susnux commented Jan 30, 2024

fixup commits are not supposed to be merged without squash

Thats what I meant: Squash the commits and add the sign off^^

Signed-off-by: Grigorii K. Shartsev <me@shgk.me>
Co-authored-by: Raimund Schlüßler <raimund.schluessler@mailbox.org>
Signed-off-by: Grigorii K. Shartsev <me@shgk.me>
Co-authored-by: Raimund Schlüßler <raimund.schluessler@mailbox.org>
@ShGKme ShGKme force-pushed the fix/nc-actions--custom-children branch from 22f02b7 to 5b951e5 Compare January 30, 2024 12:27
@ShGKme ShGKme merged commit 4a2c155 into master Jan 30, 2024
15 checks passed
@ShGKme ShGKme deleted the fix/nc-actions--custom-children branch January 30, 2024 13:25
@ShGKme
Copy link
Contributor Author

ShGKme commented Jan 30, 2024

/backport to next

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 bug Something isn't working feature: actions Related to the actions components regression Regression of a previous working feature
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants