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

fix(Heading+Label+ValidationMessage): clean up css styles #2677

Merged
merged 6 commits into from
Oct 25, 2024

Conversation

eirikbacker
Copy link
Contributor

Figma task is added as #2676

@eirikbacker eirikbacker self-assigned this Oct 24, 2024
Copy link

changeset-bot bot commented Oct 24, 2024

⚠️ No Changeset found

Latest commit: fbb2c47

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 Oct 24, 2024

Preview deployments for this pull request:

Storybook - 25. Oct 2024 - 10:08

Copy link
Contributor

github-actions bot commented Oct 24, 2024

Coverage Report

Status Category Percentage Covered / Total
🔵 Lines 55.93% 3357 / 6002
🔵 Statements 55.93% 3357 / 6002
🔵 Functions 84.84% 168 / 198
🔵 Branches 74.8% 484 / 647
File CoverageNo changed files found.
Generated in workflow #594 for commit fbb2c47 by the Vitest Coverage Report Action

@eirikbacker eirikbacker requested a review from Barsnes October 24, 2024 12:02
Comment on lines 87 to 97
&[data-size='sm'] {
@composes ds-heading-text--2xs from './base/base.css';
font-size: var(--ds-heading-2xs-font-size);
}

&[data-size='md'] {
@composes ds-heading-text--sm from './base/base.css';
font-size: var(--ds-heading-sm-font-size);
}

&[data-size='lg'] {
@composes ds-heading-text--md from './base/base.css';
font-size: var(--ds-heading-md-font-size);
}
Copy link
Member

Choose a reason for hiding this comment

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

Don't we need letter-spacing and font-weight here?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thats implying the Avatar initials should have the Heading styling as well, but design was not sure about this - font sizes was mostly used to scale with the shape. Lasse and I want to include Øyvind on this one, so it is in the todo list for #2665, but most likely we'll disconnect from heading sizes completely and rather use container query to scale font to match size :)

Copy link
Member

@Barsnes Barsnes Oct 25, 2024

Choose a reason for hiding this comment

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

Sure, but I think we should set something for those here, otherwise it will inherit and avatars may look different depending on where you put it.

Copy link
Contributor Author

@eirikbacker eirikbacker Oct 25, 2024

Choose a reason for hiding this comment

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

Thats intentional; if letter-spacing is scaled up on body-text, this should be inherited as this is probably done as an adjustment to the currently used font. font-weight is set on line 15 in avatar.css and line-height is redundant as we are using display: flex and initials will always be single line :)

@eirikbacker eirikbacker requested a review from Barsnes October 25, 2024 06:24
Copy link
Member

@Barsnes Barsnes left a comment

Choose a reason for hiding this comment

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

Avatar and it's inheritance will be looked at by design, might change, might not.

@eirikbacker eirikbacker merged commit f4f76d3 into next Oct 25, 2024
10 checks passed
@eirikbacker eirikbacker deleted the fix/simplify-heading-label-validationmessage-css branch October 25, 2024 11:41
Barsnes added a commit that referenced this pull request Oct 28, 2024
commit ce23f32
Author: Eirik Backer <eirik.backer@gmail.com>
Date:   Mon Oct 28 10:31:21 2024 +0100

    fix(Chip): use input component (#2683)

    - Fixes #2669
    - Fixes wrong height implemented (now correctly `32px`)
    - Implements simplified states after dialogue with Marianne
    - Fixes better alignment of label vs. radio/checkbox
    - Implements logic so elements with the `ds-focus--visible` class
    automatically hides focus ring on children (no need for confusing,
    nested focus rings)

commit d5e0ba1
Author: Lasse Febakke Straum <33222679+Febakke@users.noreply.github.com>
Date:   Fri Oct 25 15:08:14 2024 +0200

    refactor(tokens): changed spacing and sizing type to dimension (#2688)

    Co-authored-by: Tobias Barsnes <tobias.barsnes@digdir.no>
    Co-authored-by: Michael Marszalek <mimarz@gmail.com>

commit 6c1f99d
Author: Eirik Backer <eirik.backer@gmail.com>
Date:   Fri Oct 25 13:52:51 2024 +0200

    chore: remove tmp field styling in testing story (#2687)

    - Since field styling is now merged, `testing.stories.tsx` no longer
    needs to fake it

commit f4f76d3
Author: Eirik Backer <eirik.backer@gmail.com>
Date:   Fri Oct 25 13:41:36 2024 +0200

    fix(Heading+Label+ValidationMessage): clean up css styles (#2677)

    Figma task is added as #2676

commit d2fc6b9
Author: Tobias Barsnes <tobias.barsnes@digdir.no>
Date:   Fri Oct 25 13:16:17 2024 +0200

    feat: rename `size` prop to `data-size` (#2680)

    resolves #2673

commit acef771
Author: Eirik Backer <eirik.backer@gmail.com>
Date:   Fri Oct 25 10:24:19 2024 +0200

    fix(Input): inherit line-height (#2685)

    As pointed out by hawk-eye @mimarz 💪 🙏 🦅
    https://designsystemet.slack.com/archives/C07K7NEKXEW/p1729843841118129

commit d3c58b0
Author: Eirik Backer <eirik.backer@gmail.com>
Date:   Fri Oct 25 10:04:16 2024 +0200

    fix(Spinner): use forwardRef and aria-label for consistency (#2682)

    - Use `aria-label` for accessible spinner label to be consistent with
    other components
    - Use `forwardRef` on Spinner

    ---------

    Co-authored-by: Tobias Barsnes <tobias.barsnes@digdir.no>

commit 326671a
Author: Une Sofie Kinn Ekroll <une.kinn.ekroll@bekk.no>
Date:   Fri Oct 25 08:26:49 2024 +0200

    feat(tokens): add modes for semantic color categories main & support (#2643)

    Co-authored-by: Lasse Febakke Straum <33222679+Febakke@users.noreply.github.com>

commit 7520547
Author: Eirik Backer <eirik.backer@gmail.com>
Date:   Thu Oct 24 14:19:33 2024 +0200

    fix(Radio+Checkbox): use input component (#2607)

    - Part of #2311
    - Fieldset at compound components moved to task #2666
    - Fixes #2459

commit f96289a
Author: Tobias Barsnes <tobias.barsnes@digdir.no>
Date:   Thu Oct 24 11:17:42 2024 +0200

    chore: enable `noUnusedImports` biome rule (#2675)

    enables https://biomejs.dev/linter/rules/no-unused-imports/

commit a452813
Author: Une Sofie Kinn Ekroll <une.kinn.ekroll@bekk.no>
Date:   Thu Oct 24 10:38:59 2024 +0200

    feat(cli,theme): don't output underlying primitives for semantic color variables (#2641)
mimarz pushed a commit that referenced this pull request Feb 21, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Status: ✅ Done
Development

Successfully merging this pull request may close these issues.

None yet

2 participants