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.

Sign up for GitHub

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

Jump to bottom

fix(NcActions): correct dialog a11y attrs place #5381

Merged
merged 2 commits into from
Mar 8, 2024
Merged

fix(NcActions): correct dialog a11y attrs place #5381

merged 2 commits into from
Mar 8, 2024

Conversation

ShGKme
Copy link
Contributor

@ShGKme ShGKme commented Mar 8, 2024

☑️ Resolves

  • In dialog, role="dialog" should be not a replacement of UL

🖼️ Screenshots

🏚️ Before 🏡 After
image image

🚧 Tasks

  • Refactor: move a11y attrs to config
    • So now almost everything that changes a11y stuff depending on a11y semantic role is moved to one place
  • Move a11y attributes for dialog from UL to its container
    • role, aria-labelledby, aria-modal and id to connect with aria-controls

🏁 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

ShGKme added 2 commits March 8, 2024 11:18
@ShGKme
refactor(NcActions): move a11y attrs to config
eb173f2 
Signed-off-by: Grigorii K. Shartsev <me@shgk.me>
@ShGKme
fix(NcActions): move dialog a11y attrs from UL to container
5b35b7e 
Signed-off-by: Grigorii K. Shartsev <me@shgk.me>
@ShGKme ShGKme added bug Something isn't working 3. to review Waiting for reviews feature: actions Related to the actions components accessibility Making sure we design for the widest range of people possible, including those who have disabilities labels Mar 8, 2024
@ShGKme ShGKme added this to the 8.10.0 milestone Mar 8, 2024
@ShGKme ShGKme self-assigned this Mar 8, 2024
@ShGKme ShGKme force-pushed the fix/nc-actions--dialog-a11y-attrs branch from 92cd1ab to 5b35b7e Compare March 8, 2024 10:45
susnux
susnux reviewed Mar 8, 2024
View reviewed changes
src/components/NcActions/NcActions.vue
@@ -1126,11 +1126,18 @@ export default {
'click',
],

setup() {
const randomId = `menu-${GenRandomId()}`
Copy link
Contributor

Choose a reason for hiding this comment

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

nice! Does not need to be reactive!

@susnux susnux changed the title fic(NcActions): correct dialog a11y attrs place fix(NcActions): correct dialog a11y attrs place Mar 8, 2024
Pytal
Pytal approved these changes Mar 8, 2024
View reviewed changes
Copy link
Contributor

@Pytal Pytal left a comment

Choose a reason for hiding this comment

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

Code looks good!

@susnux susnux merged commit 147f221 into master Mar 8, 2024
18 checks passed
@susnux susnux deleted the fix/nc-actions--dialog-a11y-attrs branch March 8, 2024 10:52
This was referenced Mar 11, 2024
Merged
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Pytal Pytal Pytal approved these changes

@susnux susnux susnux approved these changes

@AndyScherzinger AndyScherzinger AndyScherzinger approved these changes

@emoral435 emoral435 Awaiting requested review from emoral435

Assignees

@ShGKme ShGKme

Labels
3. to review Waiting for reviews accessibility Making sure we design for the widest range of people possible, including those who have disabilities bug Something isn't working feature: actions Related to the actions components
Projects
None yet
Milestone
8.10.0
Development

Successfully merging this pull request may close these issues.
4 participants
@ShGKme @AndyScherzinger @susnux @Pytal