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

💄 tweak CSS for list icons #3148

Merged
merged 5 commits into from
Sep 13, 2024
Merged

Conversation

JulianNymark
Copy link
Contributor

  • on safari the ordered list items don't seem to line up well (ugly hack :( )
  • the icons (according to figma) should actually be 24px x 24px, yet they seemed to be a bit smaller (18x18), with these changes it should reflect the design closer.

fixes

  • latest feedback regarding spacing being too big between icon and text (might fix this, the apparent distance might seem smaller when the icon is bigger)
  • weird safari rendering bug with OL? (hacky)

- on safari the ordered list items don't seem to line up well (ugly hack :( )
- the icons (according to figma) should actually be 24px x 24px, yet
  they seemed to be a bit smaller (18x18), with these changes it should
  reflect the design closer.
Copy link

changeset-bot bot commented Sep 11, 2024

🦋 Changeset detected

Latest commit: 2d82337

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-css Patch
@navikt/aksel-stylelint Patch
@navikt/aksel Patch
@navikt/ds-react 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

github-actions bot commented Sep 11, 2024

Storybook demo

Endringer til review: 3

ab41c4fd7 | 90 komponenter | 141 stories

@navikt/core/css/list.css Outdated Show resolved Hide resolved
@@ -77,7 +94,7 @@
}

.navds-list ol li {
padding-left: var(--a-spacing-1);
padding-left: var(--a-spacing-1-alt);
Copy link
Contributor

Choose a reason for hiding this comment

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

This increases the spacing between the number and the text. Is that intended?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This was in the set of changes to make things look similar across all devices, there seems to be a minimum space that I can't shorten on safari :(, it's pretty close to what we currently have, so I only increased the space for every other browser with this.

We can also remove the hack code here (safari CSS) if we want the other alternative (setting content + counter())? Does it solve everything well without drawbacks? (if so, then it's much better than this one)

Copy link
Contributor

Choose a reason for hiding this comment

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

Didn't see any drawbacks when testing counter() yesterday, but need to test it properly.

@navikt/core/css/list.css Outdated Show resolved Hide resolved
@JulianNymark JulianNymark changed the title 💄 tweak CSS for list icons + safari CSS hack 💄 tweak CSS for list icons Sep 12, 2024
@JulianNymark JulianNymark marked this pull request as ready for review September 12, 2024 13:49
@HalvorHaugan
Copy link
Contributor

If you create a changeset we could probably just merge this and create a new PR for the counter() stuff. What do you think?

@JulianNymark
Copy link
Contributor Author

If you create a changeset we could probably just merge this and create a new PR for the counter() stuff. What do you think?

sounds good 👍 , this PR will only be the single change to font-size then (also some story tweaks to showcase changes better (horizontalSplitIcon is one of the larger ones, the fork is one of the smaller ones on width), title was also nice to show for alignment in that story.

@JulianNymark JulianNymark enabled auto-merge (squash) September 13, 2024 09:51
Copy link
Contributor

@HalvorHaugan HalvorHaugan left a comment

Choose a reason for hiding this comment

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

LGTM

@JulianNymark JulianNymark merged commit 057bfe8 into main Sep 13, 2024
4 checks passed
@JulianNymark JulianNymark deleted the list-tweaks-icon-and-safari branch September 13, 2024 14:17
@github-actions github-actions bot mentioned this pull request Sep 13, 2024
This pull request was closed.
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