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

Combo filteredoptions offset fix #3269

Merged
merged 2 commits into from
Oct 25, 2024
Merged

Conversation

JulianNymark
Copy link
Contributor

Description

Fixes the minor visual bug / inconsistency where when you scroll upwards, there seems to be a 1 row offset from the top of the list to the focused option. This offset is only needed in the particular instance where the 'max X of Y' maximum limit is rendered. It's sticky so it lands on top of the actual content. This is why there is a margin.

Component Checklist 📝

  • JSDoc
  • Examples
  • Documentation
  • Storybook
  • Style mappings (@navikt/core/css/config/_mappings.js)
  • Component tokens (@navikt/core/css/tokens.json)
  • CSS class deprecations (@navikt/aksel-stylelint/src/deprecations.ts)
  • Exports (@navikt/core/react/src/index.ts and @navikt/core/react/package.json)
  • New component? CSS import (@navikt/core/css/index.css)
  • Breaking change? Update migration guide. Consider codemod.
  • Changeset (Format: <Component>: <gitmoji?> <Text>. E.g. "Button: ✨ Add feature xyz.")

Copy link

changeset-bot bot commented Oct 22, 2024

🦋 Changeset detected

Latest commit: b7e5d8a

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

Copy link
Contributor

Storybook demo

5ed9d936c | 91 komponenter | 144 stories

@@ -58,7 +58,9 @@ const FilteredOptions = () => {
<ul
ref={setFilteredOptionsRef}
role="listbox"
className="navds-combobox__list-options"
className={cl("navds-combobox__list-options", {
"navds-combobox__list--max-selected": maxSelected?.isLimitReached,
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
"navds-combobox__list--max-selected": maxSelected?.isLimitReached,
"navds-combobox__list-options--max-selected": maxSelected?.isLimitReached,

Copy link
Contributor Author

Choose a reason for hiding this comment

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

ah, jeg laget en ny class for å treffe parent med max-selected 😓 (så slapp man legge den på alle items)

Copy link
Contributor

Choose a reason for hiding this comment

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

No biggie

Copy link
Contributor Author

Choose a reason for hiding this comment

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

teknisk sett følger fortsatt BEM her, bare at element blir list og ikke list-options 🤔. Er nok litt BEM-rot her som kan ryddes i også tenker jeg.

Prøvde opprinnelig å løse dette med :has() og uten kodeendring, men fant ingen løsning der.

Copy link
Contributor

Choose a reason for hiding this comment

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

Slik jeg forstår det, skal man bruke det opprinnelige navnet når man lager en modifier-klasse, her "navds-combobox__list-options". Det er dessuten allerede et element som heter "navds-combobox__list".

https://getbem.com/naming/

Modifier is an extra class name which you add to a block/element DOM node. Add modifier classes only to blocks/elements they modify, and keep the original class.

Men :has() var en god idé, det burde jo funket... 🤔

@JulianNymark JulianNymark merged commit 81c79d8 into main Oct 25, 2024
4 checks passed
@JulianNymark JulianNymark deleted the combo-filteredoptions-offset-fix branch October 25, 2024 12:52
@github-actions github-actions bot mentioned this pull request Oct 25, 2024
@HalvorHaugan
Copy link
Contributor

HalvorHaugan commented Oct 25, 2024

I see now that scroll-margin-top doesn't always work in Chrome (not caused by these changes I think). I can reproduce it here. I select the 5 first items, plus the one on the bottom. When I use arrow keys to go from the last item and up, it only scrolls on every other item:

Scrollbug.mp4

My guess is that the browser doesn't take the scroll-margin-top into account when figuring out whether the element is in view or not. It only takes it into account when actually scrolling. At least when using "nearest". It can be fixed by changing element?.scrollIntoView?.({ block: "nearest" }); to "center" instead of "nearest".

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.

3 participants