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

Bugfix/combobox bugfixes and tweaks #2144

Merged
merged 10 commits into from Aug 9, 2023

Conversation

it-vegard
Copy link
Contributor

Description

Modifications to combobox based on feedback from the summer students++:

Change summary

  • Fixed inconsistency where the input field was only cleared on enter-selecting and not "mouse click"-selecting options
  • Updated "MultiSelect with add new" example to actually use "add new" prop
  • Added flag for "isAddedByUser" to onToggleSelected, to assist the parent app in determining if the option is a new or existing option
  • Add bold text to selected options for further affordance, as the checkmark may be out-of-sight for users using screen magnication, and the selected-background-color may not be displayed on screens with incorrect color setup
  • Tweaked padding for the combobox wrapper, so the input field does not change size when adding chips/selecting options.

Makes the behaviour consistent when clicking with a mouse and when selecting an option with keyboard.
…s in combobox.

Done to account for screen magnifiers on screens that don't show the background color, where the checkmark is out-of-sight.
…he chips being slightly larger than the padding allowed for (equal to the width of the border)
@changeset-bot
Copy link

changeset-bot bot commented Aug 9, 2023

🦋 Changeset detected

Latest commit: 99f09fb

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 7 packages
Name Type
@navikt/ds-react Patch
@navikt/ds-css Patch
@navikt/aksel-stylelint Patch
@navikt/aksel Patch
@navikt/ds-tokens Patch
@navikt/ds-tailwind Patch
@navikt/aksel-icons Patch

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

@github-actions
Copy link
Contributor

github-actions bot commented Aug 9, 2023

Endringer til review: 1

a8550f9fa | 47 komponenter | 299 stories

@@ -243,6 +246,10 @@
background-color: var(--ac-combobox-list-item-selected-bg, var(--a-surface-selected));
}

.navds-combobox__list-item--selected p {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Tror den bare trengs å settes på wrapper (for lavere specificity)

Suggested change
.navds-combobox__list-item--selected p {
.navds-combobox__list-item--selected {

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Nei, .navds-body-short overstyrer den, dessverre. Var litt usikker på å bruke p eller om jeg burde bruke klassenavnet, men vi må i hvert fall ha en høyere spesifisitet enn .navds-body-short. (Arv fra parent har lavere spesifisitet)

@KenAJoh
Copy link
Collaborator

KenAJoh commented Aug 9, 2023

LGTM!
En "bug" som jeg er usikker på kan løses er klikkflaten til selve input inne i combobox

Screen.Recording.2023-08-09.at.09.23.58.mov

Har her vært litt bedre UX hvis input brukte hele høyden + ut til venstre slik at popover ikke lukket seg ved mousedown
Dette er forsåvidt en urelatert bug til denne PR-en, så er bare å forkaste å se på senere en gang 😄

…dTarget in onBlur will be null, and we can't check that focus has entirely moved outside the Combobox.
@it-vegard
Copy link
Contributor Author

Har her vært litt bedre UX hvis input brukte hele høyden + ut til venstre slik at popover ikke lukket seg ved mousedown

Jeg er enig, men da vil vi sitte med et issue med andre elementer som må "floate" (absolutt posisjonere seg?) over inputen. Kunne vært interessant å teste, men vi "faker" det ved å sette fokus i input-feltet når man klikker innenfor borderen (.combobox-wrapper).

Fant ut at buggen skyldes at sjekken i ComboboxWrapper.onBlur feiler, da e.relatedTarget blir null når man klikker på et ikke-interaktivt element. La til tabIndex="-1", så nå skal det fungere som ønskelig.

@it-vegard it-vegard merged commit eff8a90 into main Aug 9, 2023
3 checks passed
@it-vegard it-vegard deleted the bugfix/combobox-bugfixes-and-tweaks branch August 9, 2023 12:15
@github-actions github-actions bot mentioned this pull request Aug 9, 2023
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.

None yet

2 participants