-
Notifications
You must be signed in to change notification settings - Fork 51
HDS-5265: Adds a11y guardrails in Dropdown ToggleIcon component
#3087
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
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
Dropdown ToggleIcon component
Dropdown ToggleIcon componentDropdown ToggleIcon component
| Acceptable value: any [icon](/icons/library) name. | ||
| </C.Property> | ||
| <C.Property @name="hasChevron" @type="boolean" @default="true"> | ||
| Per design, `false` is only currently allowed when the "more-horizontal" or "more-vertical" icons are used; it is set to `true` by default. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In this PR, we include both more-horizontal and more-vertical in the allowed icon list, but existing documentation seems to only refer to more-horizontal being permitted without chevrons (when used as an overflow menu inTable components).
Just to check, it's okay to have more-vertical in the allowed list, and update documentation to reflect this?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We didn't have both when we first made this component, I think, but @hashicorp/hds-design could confirm.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As mentioned during our HDS Engineering sync today, better to involve the HDS designers on this, to discuss/decide which icons are "allowed" to have when the chevron is not visible. We can't make an unilateral decision on this (even if it's documented, better to double check with them).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes agreed! Just marked the PR for review so everyone can discuss :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for looping @hashicorp/hds-design in! I think this change is logical given how we expect this component to be used, and I can't think of any other icons that would fall into this category at this time.
However, where this gives me pause is whether this is a guidance over control question. The reasoning behind supporting this variant when used in the table is because this is a common pattern across the web for consolidating options and CRUD actions in dense UIs, and while I can't think of other icon examples right now, I imagine there are others that are logical given the context.
I'll invite other designers to chime in with opinions, we discussed this briefly today, but were missing a few folks that would benefit the discussion. I might request holding off until after our design sync later this week for us to discuss it with a full quorum.
3c789df to
fb70667
Compare
π¦ RC Packages PublishedLatest commit: fb70667 |
f6ef577 to
5a7d876
Compare
4cc8bce to
a8d64c1
Compare
feat: update enum and type names for allowed icons tests pass (#3089) Co-authored-by: Anna Wang <[email protected]> Failing test fixed and reworked the hasChevron() function to more closely align with the syntax of other functions feat: update docs to mention more-vertical as well feat: added changeset fix: edited changeset formatting to be more consistent fix: type of allowed icons array fix: website indentation issue
a8d64c1 to
69ee082
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
π π
π Summary
If merged, this PR adds a11y guardrails to the
DropdownToggleIconcomponent, so the consumer can only set@hasChevronto false if the icon is in a list of allowed values.π οΈ Detailed description
@hasChevronπΈ Screenshots
Tests pass.


Smoke testing
@hasChevron={{false}}with an invalid iconπ External links
Jira ticket: HDS-5265
π Component checklist
π¬ Please consider using conventional comments when reviewing this PR.
π PCI review checklist
Examples of changes to controls include access controls, encryption, logging, etc.
Examples include changes to operating systems, ports, protocols, services, cryptography-related components, PII processing code, etc.