Combobox: add translations and adjust buttons #3409
Conversation
🦋 Changeset detectedLatest commit: 0896b88 The changes in this PR will be included in the next version bump. This PR includes changesets to release 7 packages
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
| Combobox: { | ||
| addOption: "Add", | ||
| searching: "Searching…", | ||
| maxSelected: "{selected} of max {limit} are selected.", |
There was a problem hiding this comment.
I took the liberty of adding "max" to the text to make it clearer.
Storybook demo / Chromatic📝 Endringer til review: 2e502b415c | 91 komponenter | 135 stories |
| > | ||
| <span className="navds-sr-only"> | ||
| {toggleListButtonLabel ?? "Alternativer"} |
There was a problem hiding this comment.
"Alternativer" doesn't tell me what the action does here. Would "open alternatives", "open optionlist" or something in that world make sense?
There was a problem hiding this comment.
I don't think we need this text, so I just removed it and deprecated toggleListButtonLabel. No one is currently using that prop, so maybe we can just remove it right away?
There was a problem hiding this comment.
@it-vegard @KenAJoh Should we remove the prop toggleListButtonLabel entirely instead of just marking it as deprecated?
There was a problem hiding this comment.
Do it! Love removing unused props where possible ❤️
There was a problem hiding this comment.
@it-vegard Please have a look at the event changes I made here. I removed onKeyDown since that shouldn't work anyways. I also changed onPointerUp to onClick since it should work the same way, but is better practice. Do you think this is ok?
| @@ -88,10 +94,9 @@ export const InputController = forwardRef< | |||
| onClick={clearInput} | |||
| className="navds-combobox__button-clear" | |||
| tabIndex={-1} | |||
| aria-hidden | |||
There was a problem hiding this comment.
aria-hidden on button results in this error message in the console in Chrome:
Blocked aria-hidden on an element because its descendant retained focus. The focus must not be hidden from assistive technology users. Avoid using aria-hidden on a focused element or its ancestor. Consider using the inert attribute instead, which will also prevent focus. For more details, see the aria-hidden section of the WAI-ARIA specification at https://w3c.github.io/aria/#aria-hidden.
Element with focus: button
Wondering if I should use a div instead? 🤔
There was a problem hiding this comment.
Since this and ToggleListButton are both aria-hidden, can the wrapper-div get aria-hidden to solve this?
There was a problem hiding this comment.
Is there actually a reason to hide this element? In Search we opt to not hide it, so is there an actual difference in expected UX here?
Component Checklist 📝
@navikt/core/css/config/_mappings.js)@navikt/core/css/tokens.json)@navikt/aksel-stylelint/src/deprecations.ts)@navikt/core/react/src/index.tsand@navikt/core/react/package.json)@navikt/core/css/index.css)<Component>: <gitmoji?> <Text>.E.g. "Button: ✨ Add feature xyz.")