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

TINY-10794: Remove role="listbox" attribute from dropdowns #9571

Closed
wants to merge 3 commits into from

Conversation

vpyshnenko
Copy link
Contributor

@vpyshnenko vpyshnenko commented Apr 18, 2024

Related Ticket: TINY-10794

Description of Changes:

  • Removed hardcoded role="listbox" from dropdowns

Pre-checks:

  • Changelog entry added
  • Tests have been added (if applicable)
  • Branch prefixed with feature/, hotfix/ or spike/

Review:

  • Milestone set
  • Docs ticket created (if applicable)

GitHub issues (if applicable):

@vpyshnenko vpyshnenko self-assigned this Apr 18, 2024
@vpyshnenko vpyshnenko added this to the 7.1.0 milestone Apr 18, 2024
@@ -245,7 +245,7 @@ const makeSandbox = (
// TODO: Add aria-selected attribute
attributes: {
id: ariaControls.id,
role: 'listbox'
...detail.role?.fold( Fun.constant({}), (role) => ({ role })) ?? {}
Copy link
Member

Choose a reason for hiding this comment

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

Why do we have an optional field with an Optional value 🤦

please either make the field mandatory or the value unwrapped.

If there's really no other way... you can actually spread undefined. It feels dirty though.

Suggested change
...detail.role?.fold( Fun.constant({}), (role) => ({ role })) ?? {}
...detail.role?.map((role) => ({ role })).getOr({})

@vpyshnenko
Copy link
Contributor Author

Closed due the changes are incorporated in the other PR #9665
Specifically in

removed listbox role. Since it is a alloy sandbox wrapper there's no need to pass the role to it.

@vpyshnenko vpyshnenko closed this May 23, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants