-
Notifications
You must be signed in to change notification settings - Fork 42
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
Combobox: 🐛 implement Readonly + disabled #3180
Conversation
🦋 Changeset detectedLatest commit: 7b64a6f 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 |
Storybook demoEndringer til review: 16699cad12 | 90 komponenter | 141 stories |
@navikt/core/react/src/form/combobox/SelectedOptions/SelectedOptions.tsx
Show resolved
Hide resolved
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.
Consider adding aria-readonly
to the input field (Looks like role="combobox"
overrides til semantics of the readOnly
prop)
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.
I've tried setting the readOnly
native html attribute (which is preferred to aria-readonly
, but even with and without the role="combobox" attribute, it seems to not be presented to screenreaders as a read-only input? 🤔
I would like something like this:
<input type="text" role="combobox" readOnly />
Need to do a bit more experimentation to find a combination of role, type and readonly / aria-readonly that works.
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.
So maybe support for readonly is just a bit lackluster currently? 😞, at least we now set it for future support, and readonly is read up from the title on the readonlyIcon.
…veReadOnly={false}
This reverts commit e54a2ec.
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.
LGTM 🚀
Description
Both
readonly
anddisabled
were not working for Combobox. This PR aims to implement both of those in one fell swoop (mostly since they are similar-ish features).Closes: https://github.com/navikt/team-aksel/issues/645
Closes: #3049
Component Checklist 📝
@navikt/core/css/config/_mappings.js
)@navikt/core/css/tokens.json
)@navikt/aksel-stylelint/src/deprecations.ts
)@navikt/core/react/src/index.ts
and@navikt/core/react/package.json
)@navikt/core/css/index.css
)<Component>: <gitmoji?> <Text>.
E.g. "Button: ✨ Add feature xyz.")