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

chore(css): go over and make consistent, highlight issues #3002

Merged
merged 17 commits into from
Jan 21, 2025

Conversation

Barsnes
Copy link
Member

@Barsnes Barsnes commented Jan 16, 2025

#2638

Highlights:

  • Sometimes we use a var -color, and then use it for background:. In these cases it is to add color to an icon - so the usage is correct. badge.css is an example of this.
  • We use the word spacing in our tokens quite a bit. This is reserved for where we use it to do more than just set a property. For example in padding, but also in a calc for margin-inline
  • We were incosistent in placement of css props on the root classname. I moved this to just after the initial css var definitions.
  • Where we have used vars for icons, all of them have been changed to use -icon. Details had chevron, for example. Search is an exception to this, as it has multiple icons. Both of them are named in this case,
  • We only use background, not background-color
  • border: now uses two properties. The color is a variable, and the width and style is a css prop, unless used multiple places in the same component. border and border-color will still stay as separate vars

combobox.css has not been touched

Copy link

changeset-bot bot commented Jan 16, 2025

⚠️ No Changeset found

Latest commit: bd8d2eb

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

Copy link
Contributor

github-actions bot commented Jan 16, 2025

Preview deployments for this pull request:

Storybook - 20. Jan 2025 - 16:24

Copy link
Contributor

github-actions bot commented Jan 16, 2025

Coverage Report

Status Category Percentage Covered / Total
🔵 Lines 53.41% 2955 / 5532
🔵 Statements 53.41% 2955 / 5532
🔵 Functions 86.12% 180 / 209
🔵 Branches 76.79% 523 / 681
File CoverageNo changed files found.
Generated in workflow #1691 for commit bd8d2eb by the Vitest Coverage Report Action

@Barsnes Barsnes marked this pull request as ready for review January 17, 2025 08:29
Copy link
Contributor

@eirikbacker eirikbacker left a comment

Choose a reason for hiding this comment

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

Nice work!! 🙏 ❤️ ser det er inconsistencies (fra tidligere), og nok ting vi burde snakke gjennom 3 stk så vi tester logikken på hverandre for å lande hva som funker best ☺️

padding-block: var(--dsc-alert-padding);
padding-inline: calc(var(--dsc-alert-padding) + var(--dsc-alert-icon-size) + var(--dsc-alert-gap)) var(--dsc-alert-padding);
padding-block: var(--dsc-alert-padding-block);
padding-inline: calc(var(--dsc-alert-padding-block) + var(--dsc-alert-icon-size) + var(--dsc-alert-spacing)) var(--dsc-alert-padding-block);
Copy link
Contributor

Choose a reason for hiding this comment

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

Her er padding-block brukt til padding-inline, og dersom man sender inn padding-block: 1rem 2rem (som er mulig i vanlig padding-block) så brekker det. Skulle vi ha funnet på enda et ord her?
eller evt. gjort om så det er:

--dsc-alert-padding => --dsc-alert-spacing
--dsc-alert-gap => --dsc-alert-icon-spacing

Copy link
Member Author

Choose a reason for hiding this comment

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

For å sitere meg sjølv:
border: now uses two properties. The color is a variable, and the width and style is a css prop.

Denne gjelder der samme border ikkje er brukt fleire plassa. table fks, burde ha eigen variabel for border:

Copy link
Member Author

Choose a reason for hiding this comment

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

Ser ut som eg har greid å lagt denne kommentaren på feil plass, eg skal sjekke dette ut.

Copy link
Member Author

Choose a reason for hiding this comment

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

Eg har endra denne til å bruke eksplisitte css props, Det vil no sjå slik ut:

  --dsc-alert-padding-block: var(--ds-size-6);
  --dsc-alert-padding-inline-end: var(--ds-size-6);
  
  padding-block: var(--dsc-alert-padding-block);
  padding-inline-start: calc(var(--dsc-alert-padding-inline-end) + var(--dsc-alert-icon-size) + var(--dsc-alert-spacing));
  padding-inline-end: var(--dsc-alert-padding-inline-end);

Eg er ikkje veldig fornøgd med løysingen, men det er bedre.

packages/css/src/card.css Outdated Show resolved Hide resolved
packages/css/src/card.css Outdated Show resolved Hide resolved
packages/css/src/details.css Show resolved Hide resolved
packages/css/src/details.css Outdated Show resolved Hide resolved
packages/css/src/list.css Show resolved Hide resolved
packages/css/src/modal.css Outdated Show resolved Hide resolved
packages/css/src/modal.css Outdated Show resolved Hide resolved
packages/css/src/popover.css Show resolved Hide resolved
packages/css/src/validation-message.css Show resolved Hide resolved
@eirikbacker eirikbacker merged commit 10cb65f into next Jan 21, 2025
10 checks passed
@eirikbacker eirikbacker deleted the css/consistency branch January 21, 2025 08:31
mimarz pushed a commit that referenced this pull request Jan 21, 2025
#2638

## Highlights:
- Sometimes we use a var `-color`, and then use it for `background:`. In
these cases it is to add color to an icon - so the usage is correct.
`badge.css` is an example of this.
- We use the word `spacing` in our tokens quite a bit. This is reserved
for where we use it to do more than just set a property. For example in
padding, but also in a calc for `margin-inline`
- We were incosistent in placement of css props on the root classname. I
moved this to just after the initial css var definitions.
- Where we have used vars for icons, all of them have been changed to
use `-icon`. `Details` had `chevron`, for example. `Search` is an
exception to this, as it has multiple icons. Both of them are named in
this case,
- We only use `background`, not `background-color`
- `border:` now uses two properties. The color is a variable, and the
width and style is a css prop, unless used multiple places in the same
component. `border` and `border-color` will still stay as separate vars

**`combobox.css` has not been touched**
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