Combobox: Fix issue where using arrow keys in list would make the entire page scroll

navikt / aksel

NAVs designsystem og Aksel-portalen

https://aksel.nav.no

MIT License

158 stars 42 forks source link

Combobox: Fix issue where using arrow keys in list would make the entire page scroll #3364

Closed HalvorHaugan closed 1 week ago

HalvorHaugan commented 1 week ago

Component Checklist 📝

[ ] JSDoc
[ ] Examples
[ ] Documentation / Decision Records
[ ] 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.
[x] Changeset (Format: <Component>: <gitmoji?> <Text>. E.g. "Button: :sparkles: Add feature xyz.")

changeset-bot[bot] commented 1 week ago

🦋 Changeset detected

Latest commit: 9ff0bf003e0640b5dc87e2fd2f133009d5bd9b46

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[bot] commented 1 week ago

Storybook demo

0d58f9f20 | 91 komponenter | 138 stories

it-vegard commented 1 week ago

Seems to work. But this is reverting some changes done recently, isn't it? Do we now break something those changes fixed?

And is it possible to write a test that catches this problem? I've been thinking about adding a test case with more content exactly for these types of issues, so maybe this is the time for it?

HalvorHaugan commented 1 week ago

@it-vegard Yes, it reverts this: https://github.com/navikt/aksel/pull/3299 But setting scroll-margin-top to 48px instead of 50px seems to be a better fix for that issue than changing scrollIntoView.

HalvorHaugan commented 1 week ago

@it-vegard I will investigate if it's possible to catch it in a test. I think it should be possible in a Storybook interaction test, but I'm having doubts about vitest. But maybe Storybook is sufficient if we manage to run those tests in CI 🤔