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

Combobox: update CSS to use new tokens #3328

Merged
merged 30 commits into from
Nov 21, 2024
Merged

Conversation

JulianNymark
Copy link
Contributor

Description

closes https://github.com/navikt/team-aksel/issues/668

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.
  • Changeset (Format: <Component>: <gitmoji?> <Text>. E.g. "Button: ✨ Add feature xyz.")

@JulianNymark JulianNymark requested a review from a team as a code owner November 5, 2024 13:55
Copy link

changeset-bot bot commented Nov 5, 2024

⚠️ No Changeset found

Latest commit: c8d4236

Merging this PR will not cause a version bump for any packages. If these changes should not result in a new version, you're good to go. If these changes should result in a version bump, you need to add a changeset.

This PR includes no changesets

When changesets are added to this PR, you'll see the packages that this PR includes changesets for and the associated semver types

Click here to learn what changesets are, and how to add one.

Click here if you're a maintainer who wants to add a changeset to this PR

@JulianNymark
Copy link
Contributor Author

JulianNymark commented Nov 5, 2024

Think I need some help later with scrollbar-gutter styling... 🤔 ... this could look better
image

EDIT: done

@JulianNymark JulianNymark requested a review from a team as a code owner November 5, 2024 13:59
Copy link
Contributor

github-actions bot commented Nov 5, 2024

Storybook demo

1c82e961b | 91 komponenter | 135 stories

@navikt/core/css/darkside/form/combobox.darkside.css Outdated Show resolved Hide resolved
@navikt/core/css/darkside/form/combobox.darkside.css Outdated Show resolved Hide resolved
@navikt/core/css/darkside/form/combobox.darkside.css Outdated Show resolved Hide resolved
@navikt/core/css/darkside/form/combobox.darkside.css Outdated Show resolved Hide resolved
@navikt/core/css/darkside/form/combobox.darkside.css Outdated Show resolved Hide resolved
@navikt/core/css/darkside/form/combobox.darkside.css Outdated Show resolved Hide resolved
@navikt/core/css/darkside/form/combobox.darkside.css Outdated Show resolved Hide resolved
@navikt/core/css/darkside/form/combobox.darkside.css Outdated Show resolved Hide resolved
@navikt/core/css/darkside/form/combobox.darkside.css Outdated Show resolved Hide resolved
@JulianNymark
Copy link
Contributor Author

image

from 441 lines of CSS to.... 🥁🥁🥁 452 womp womp 🎺 (I think the actual character count might be lower though 🙏 )

@JulianNymark
Copy link
Contributor Author

JulianNymark commented Nov 6, 2024

from 441 lines of CSS to.... 🥁🥁🥁 452 womp womp 🎺 (I think the actual character count might be lower though 🙏 )

yupp, confirmed some CSS savings 💰
image
lines ⬆️ by 11, characters ⬇️ by 18

This is despite the fact that our new prefix is 1 character longer (--a vs --ax), so every place we use a variable is already at a disadvantage 😂 (also, depends on the semantic words chosen etc)

@navikt/core/css/darkside/form/combobox.darkside.css Outdated Show resolved Hide resolved
@navikt/core/css/darkside/form/combobox.darkside.css Outdated Show resolved Hide resolved
.navds-combobox--error {
.navds-combobox__wrapper {
border-radius: var(--ax-border-radius-medium);
outline: 3px solid var(--ax-border-danger-strong);
Copy link
Collaborator

Choose a reason for hiding this comment

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

Error "border" is 2px. We have solved this by setting border-color to danger and adding a 1px box-shadow with danger. This allows creation of a 2px "border" without changing border-width, and allowing regular focus with "outline" to remain --ax-border-focus.
Screenshot 2024-11-07 at 11 26 16
Screenshot 2024-11-07 at 11 26 18

Copy link
Contributor Author

@JulianNymark JulianNymark Nov 7, 2024

Choose a reason for hiding this comment

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

matching the border color with the outline / box shadow should work for this yeah. We still want to use outline > box-shadow right (in general)?

Copy link
Collaborator

Choose a reason for hiding this comment

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

The change from box-shadow -> outline is mainly for focus-handling. For other use-cases (like the error-border) box-shadow is fine i think 👍

Copy link
Contributor Author

Choose a reason for hiding this comment

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

hmm, men er litt fan av å bare ha "one thing" her
image
må holde to ting in sync (kan skape en del slike visuelle bugs med 2 borders), og mer CSS å skrive (laste ned, parse) 🤔. Setter man bare fargen til outline på error == ganske KISS, og er vel ikke teknisk sett feil? 🤔 (jeg har også en distaste for box-shadow pga subtle floating point math som er litt forskjellig på tvers av browsere... er kanskje det samme for outline? 😳 )

Copy link
Collaborator

@KenAJoh KenAJoh Nov 7, 2024

Choose a reason for hiding this comment

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

But how would focus be handled then, focus should always be --ax-border-focus-color where possible 🤔

Copy link
Collaborator

Choose a reason for hiding this comment

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

This would be fine right? (you can't have both error and focus outlines at the same time in this case, but that's usually fine, since when you're entering a value, it doesn't need to be red to draw our attention anymore 🤔 )

I think it could matter since change of color when you are in an error-state often indicates that the error is gone. Made a demo with the two version here, should probably sync this with the designers since this will be the case for textfield, textarea, select etc also

Copy link
Contributor Author

Choose a reason for hiding this comment

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

did some exploratory changes to that demo https://play.tailwindcss.com/ax4OXSQvO6?file=css, here I set negative offsets to outline-offset to have it "fully cover" the border at rest. I made the original border green so you can see the changes, (in your demo the width of the leftmost one would "shrink" as half of it moved away.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Might be missing something, but input[data-v="1"] uses a 2px border-width in the new example, something we can't do in code since it will cause shifts in layout. We can take this discussion in a huddle if that's faster 📞

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, then i'll leave it the way it is (1px border that turns red when error, 1px box shadow red when error) + 2px outline blue with offset 2 due to focus. The same as v2 in the example. 👍

Copy link
Collaborator

Choose a reason for hiding this comment

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

@navikt/core/css/darkside/form/combobox.darkside.css Outdated Show resolved Hide resolved
@navikt/core/css/darkside/form/combobox.darkside.css Outdated Show resolved Hide resolved
@navikt/core/css/darkside/form/combobox.darkside.css Outdated Show resolved Hide resolved
@navikt/core/css/darkside/form/combobox.darkside.css Outdated Show resolved Hide resolved
@JulianNymark
Copy link
Contributor Author

  • disabled & readonly hover shouldn't change border color now
  • fix error border color out of sync (on active and mouse outside)

@JulianNymark JulianNymark changed the title WIP: Combobox: update CSS to use new tokens Combobox: update CSS to use new tokens Nov 11, 2024
@JulianNymark
Copy link
Contributor Author

image
Seems like Chip needs a disabled state CSS for forced-colors too (don't want artificially increase specificity in Combobox css just because it uses another component)

@navikt/core/css/darkside/form/combobox.darkside.css Outdated Show resolved Hide resolved
@navikt/core/css/darkside/form/combobox.darkside.css Outdated Show resolved Hide resolved
.navds-combobox--error {
.navds-combobox__wrapper {
border-radius: var(--ax-border-radius-medium);
outline: 3px solid var(--ax-border-danger-strong);
Copy link
Collaborator

Choose a reason for hiding this comment

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

@JulianNymark
Copy link
Contributor Author

Might need to be fixed before merge: https://www.figma.com/design/ND5PkXtAe80PZDZQaDsvjj?node-id=11-1299&m=dev#1022025089

The figma links seems to not go to a specific element, but I reckon it's the new one with error message right?
image

@KenAJoh
Copy link
Collaborator

KenAJoh commented Nov 18, 2024

Might need to be fixed before merge: https://www.figma.com/design/ND5PkXtAe80PZDZQaDsvjj?node-id=11-1299&m=dev#1022025089

The figma links seems to not go to a specific element, but I reckon it's the new one with error message right?

Yes was a link to the figma-comment 👍 Can see how its implemented in the TextField-PR for reference if that helps
https://66b4b3beb91603ed0ab5c45e-scqspsname.chromatic.com/?path=/story/ds-react-textfield--with-error&globals=mode:darkside

@JulianNymark JulianNymark merged commit 39d83bf into main Nov 21, 2024
4 checks passed
@JulianNymark JulianNymark deleted the darkside-combobox-updated branch November 21, 2024 09:32
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.

2 participants