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: Update item count ARIA for grouped items (fixes #160) #162

Merged
merged 4 commits into from
Sep 13, 2023

Conversation

kirsty-hames
Copy link
Contributor

Fixes #160

Item count is relevant to it's parent container (all items within Boxmenu as default, or items contained within a parent group).

Test PR
Configure your course to use the Boxmenu grouping (see example for ref).

Inspect 'View' Button aria-label and item count should be relevant to the Boxmenu group set up.
e.g. item index in group + total items in group

@swashbuck
Copy link
Contributor

@kirsty-hames Thanks for taking a look at this one.

This may be intended behavior, but here my test case and what I'm seeing:

  • I'm using the standard sandbox build (Presentation Components, Question Components, and Assessment topics)
  • I've put Presentation and Question topics into a group (co-000). The "view" buttons here correctly display "1 of 2" and "2 of 2"
  • Assessment is ungrouped. The weird thing is that the "view" button is displaying "Item 2 of 2." Is this because the co-000 group is considered an item? Is this how we want it to work (I don't have any better ideas atm lol)?

@kirsty-hames
Copy link
Contributor Author

@kirsty-hames Thanks for taking a look at this one.

This may be intended behavior, but here my test case and what I'm seeing:

  • I'm using the standard sandbox build (Presentation Components, Question Components, and Assessment topics)
  • I've put Presentation and Question topics into a group (co-000). The "view" buttons here correctly display "1 of 2" and "2 of 2"
  • Assessment is ungrouped. The weird thing is that the "view" button is displaying "Item 2 of 2." Is this because the co-000 group is considered an item? Is this how we want it to work (I don't have any better ideas atm lol)?

Hey @swashbuck, yep, groups are considered an item and this is the behaviour I'd expect. I think most use cases, all items would be assigned to a group or no groups enabled (as standard Boxmenu setup). In the instance not all items are assigned a group (as per your test case), I'm not sure what an alternative would be. The current aria reflects the menu item structure at least.

js/BoxMenuGroupView.js Outdated Show resolved Hide resolved
@kirsty-hames
Copy link
Contributor Author

Thanks @swashbuck. I've applied your suggestion in a4cbeda

Copy link
Contributor

@swashbuck swashbuck left a comment

Choose a reason for hiding this comment

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

👍

js/BoxMenuGroupView.js Outdated Show resolved Hide resolved
Copy link
Contributor

@joe-allen-89 joe-allen-89 left a comment

Choose a reason for hiding this comment

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

Working as expected with FW v5.31.23

@joe-allen-89 joe-allen-89 merged commit f3ea395 into master Sep 13, 2023
@joe-allen-89 joe-allen-89 deleted the issue/160 branch September 13, 2023 10:56
github-actions bot pushed a commit that referenced this pull request Sep 13, 2023
## [6.3.12](v6.3.11...v6.3.12) (2023-09-13)

### Fix

* Update item count ARIA for grouped items (fixes #160) (#162) ([f3ea395](f3ea395)), closes [#160](#160) [#162](#162)
@github-actions
Copy link

🎉 This PR is included in version 6.3.12 🎉

The release is available on GitHub release

Your semantic-release bot 📦🚀

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Button aria text partially broken when using groups
4 participants