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

Preparations for CSS-package #2295

Closed
38 tasks done
Tracked by #1045
eirikbacker opened this issue Aug 20, 2024 · 12 comments
Closed
38 tasks done
Tracked by #1045

Preparations for CSS-package #2295

eirikbacker opened this issue Aug 20, 2024 · 12 comments
Labels
css @digdir/designsystemet-css ☂️ epic Issues ready

Comments

@eirikbacker
Copy link
Contributor

eirikbacker commented Aug 20, 2024

After meeting with @Barsnes @mimarz @Thunear @eirikbacker 26.08.24:

  • We use nesting
  • We use data-[property]="value" instead of class names for variants
  • We use aria- and pseudo selectors when relevant
  • We avoid position: relative and change of display unless necessary
  • We try to use a single class name for components if DOM structure allows it
  • We try to use css custom properties --dsc-link-color--hover for state change instead changing of css properties directly
  • We move to CSS modules composes for font setup when feat: css modules #2318 is merged, but use font-size with comment in the meantime
  • We move to CSS modules composes for focus setup when feat: css modules #2318 is merged, but use --ds-focus-boxShadow in the meantime
  • We use --dsc-button-background--hover with double dashes and state in the end for state based css custom properties
  • We use pseudo elements with mask: url(svg...) instead of inline SVGs when graphic is part of the component
  • We add default styling to base css classname, instead of requiring i.e. data-size="md" being set on every DOM element

To be done after changes

Previous discussion topics

See previous discussion topics ### Description

Starting an issue to gather toughs:

Decrease amount of class names for each tag:

  • Current implementation: Many class names are needed on the same HTML element, for example <button class="ds-btn ds-paragraph ds-paragraph--md ds-line-height--sm ds-focus ds-btn--md ds-btn--primary ds-btn--accent">.
  • Suggested implementation: Add one class name: ds.btn (imported as CSS module)
  • Discussion:
    • Defaults: The default size ds-btn-md - should this styling just be part of ds-btn? My feeling is yes: If a consumer wants to create a "variation" of the default or md, they can easily extend with their own class names, rather than needing to add multiple styling props to every element.
    • Class vs. data-attributes: See Use data attributes for css #2065
    • States: I suggest we style state based on attributes and pseudo selectors whenever possible, to avoid the need for adding and removing class names/data-attributes (i.e. .ds-btn:disabled) instead of ds-btn--disabled).

Decrease amount of class names for each component:

  • Current implementation: Class names are needed on multiple elements, for example:
<table class="ds-paragraph ds-paragraph--md ds-line-height--md ds-table ds-table--md ds-paragraph--md">
    <thead class="ds-table__head">
        <tr class="ds-table__row">
            <th class="ds-table__header__cell ds-font-weight--medium">Header 1</th>
        </tr>
    </thead>
    <tbody>
        <tr class="ds-table__row">
            <td class="ds-table__cell">Content</td>
        </tr>
    </tbody>
</table>
  • Suggested implementation: When DOM structure is dictated by HTML standards, adding multiple class names should not be necessary - we can instead add child selectors, allowing markup with pure tags:
<table class="ds-table">
    <thead>
        <tr><th>Header 1</th></tr>
    </thead>
    <tbody>
        <tr><td>Content</td></tr>
    </tbody>
</table>
  • Discussion:
    • Nesting: This means we should write .ds-table > thead > tr > th to ensure we only style the current table, and not all child th elements regardless of nesting level.
    • Non-DOM-standards: There will be scenarios where the DOM specification is less strict - should we follow the current BEM pattern then? If moving to CSS modules, class names with hyphens are a bit pain (as this requires the syntac styles['table--zebra']), but underscore is okay (styles.table__expander).

Decrease amount of DOM:

  • Current implementation: Some components have redundant amount of HTML elements, example:
<button type="button" aria-pressed="false" class="ds-focus ds-chip--button ds-chip--md">
  <span class="ds-paragraph ds-paragraph--md ds-line-height--sm ds-chip__label">
    <span>Nynorsk</span>
  </span>
</button>
  • Suggested implementation: Strive to only require essential HTML elements - both for simpler and faster DOM, and for more 1:1 interface between React tags and HTML-tags:
<button type="button" class="ds-chip">Nynorsk</button>
  • Discussion:
    • Typography: Moving to this technique, we need to avoid using <Paragraph asChild> as extensively as we do now, and rather ensure this styling is part of the CSS instead. With good use of font-tokens, this can be advantageous as each CSS file is a more concrete listing of all applied properties, but also increases file size a tiny bit.

Avoid display: flex OUTDATED, SEE COMMENT

  • Current implementation: Some components use display: flex or display: inline-flex around text elements. This makes it easy to add gap, but can have unwanted side effects if adding textual child elements, as flex causes gap and block rendering for all all child elements, example:
<Button><Star /> Text is <em>special</em></Button>

..will cause rendering: ⭐️ Text is *special*

  • Suggested implementation: Avoid display: flex and rather use display: inline-block + vertical-align and margin-left or margin-right based on :first-child or :last-child. This is a bit more CSS, but makes the implementation more robust.
  • Discussion:
    • Alternative: We can keep flex, but require text to wrapped: <Button><Star /> <span>Text is <em>special</em></span></Button>. I would not recommend this.

Avoid position: relative et.al.

  • Current implementation: Some components use position: relative without any obvious reason or comment about why. - Suggested implementation: We should avoid causing new stacking contexts whenever possible for a predictable styling environment, and comment/document places where position (or other stacking context properties) are used.

Use ::before or ::after with mask: url(svg) for component graphics

  • Current implementation: Some components use SVG elements for component graphics such as the chevron in Accordion, example:
<button type="button" class="ds-accordion__button ds-focus" aria-expanded="false" aria-controls=":r0:">
  <svg xmlns="http://www.w3.org/2000/svg" width="1em" height="1em" fill="none" viewBox="0 0 24 24" focusable="false" role="img" aria-hidden="true" class="ds-accordion__expand-icon" font-size="1.5rem"><path fill="currentColor" fill-rule="evenodd" d="M5.97 9.47a.75.75 0 0 1 1.06 0L12 14.44l4.97-4.97a.75.75 0 1 1 1.06 1.06l-5.5 5.5a.75.75 0 0 1-1.06 0l-5.5-5.5a.75.75 0 0 1 0-1.06" clip-rule="evenodd"></path></svg>
  <span class="ds-paragraph ds-paragraph--sm ds-line-height--md">Hvem kan registrere seg i Frivillighetsregisteret?</span>
</button>
  • Suggested implementation: We should avoid requiring our consumers to add these SVG elements manually, and instead render them with ::before or ::after elements.
  • Discussion:
    • Component graphics vs. icons: Icons that are not part of the component, but rather a added element (such as icons in Buttons) should still be SVG elements - only component graphics should be CSS inlined. But there will be edge cases such as the icon in Alerts - is this a component graphic, or not?

Direct styling of input elements

  • Current implementation: Input elements are styled by adding a wrapping element around both label and input, where the outermost element is the component itself, example:
<div class="ds-paragraph ds-paragraph--md ds-line-height--md ds-textfield ds-textfield--md">
  <label class="ds-label ds-label--md ds-font-weight--medium ds-textfield__label" for="textfield-:rga:">
    <span>Label</span>
  </label>
  <div class="ds-textfield__field">
    <input class="ds-textfield__input ds-focus" type="text" size="20" id="textfield-:rga:">
  </div>
  <div class="ds-textfield__error-message" id="textfield-error-:rga:" aria-live="polite" aria-relevant="additions removals"></div>
</div>
  • Suggested implementation: We can style the input element directly with a class name ds-input, and rather separate out the wrapper styling (if needed) as a own class ds-form-field so it is more modular/composable, works more closely with HTML-element names, and so that the wrapper can be re-used for Select, Datepicker, Combobox, Textarea, Textfield, or even custom form controls our consumers create. If prefix/suffix is needed, this can be an optional wrapper-element - just like the Danish Designsystem is solving it: https://designsystem.dk/komponenter/inputfelter/

Spacing as responsibility of parent element

  • Current implementation: Some elements have props to set margin/spacing
  • Suggested implementation: Instead of elements (especially typographic elements) having outer margin, we should outsource spacing to the parent element. See more on https://kyleshevlin.com/no-outer-margin/

Tasks

  1. css
    Barsnes

Conversion to this format have been done on:

Create a PR with changes and just use non keyword to link to this issue #2295.

Components

@eirikbacker eirikbacker added the css @digdir/designsystemet-css label Aug 20, 2024
@eirikbacker eirikbacker self-assigned this Aug 20, 2024
@eirikbacker eirikbacker moved this from 🔵 Inbox to ☂ Epics in Team Design System Aug 20, 2024
@Barsnes
Copy link
Member

Barsnes commented Aug 20, 2024

Some quick thoughs:

Decrease amount of DOM:

In the chip you have used as an example here, the wrappers are for icons, and to ensure children are in a singular span, since users will most likely send in pure text here. How would you solve this otherwise? We should be able to drop the outher chip__label span, but I think we need the one around children.

Avoid display: flex

I like the alternative, since using selectors like :first-child and :last-child will have more side effects, such as targeting wrong elements, if there are more than 3 elements.

Avoid position: relative et.al.

Agreed, we need it most places where it is set, but I see some examples where this should not be needed.

Use ::before or ::after with mask: url(svg) for component graphics

Agreed, this has been on our radar, but have fallen through the cracks. We have already done it for some elements, such as NativeSelect.

Direct styling of input elements

This is done to the input so we can adde prefixes and suffixes, not sure how you would solve this without a wrapper?

Spacing as responsibility of parent element

This will unfortunatley not work in some contexts, where users will need to add it manually themselves. I suggest having a parent element as an alternative, but not the only option.

@mimarz mimarz added the ☂️ epic Issues ready label Aug 20, 2024
@eirikbacker
Copy link
Contributor Author

Decrease amount of DOM:
In the chip you have used as an example here, the wrappers are for icons, and to ensure children are in a singular span, since users will most likely send in pure text here. How would you solve this otherwise? We should be able to drop the outher chip__label span, but I think we need the one around children.

  • But could we then solve icons the same way for buttons and chips? See next comment:

Avoid display: flex
I like the alternative, since using selectors like :first-child and :last-child will have more side effects, such as targeting wrong elements, if there are more than 3 elements.

  • Not necessarily - .ds-btn > svg:first-child is a pretty safe selector? :)

Direct styling of input elements
This is done to the input so we can adde prefixes and suffixes, not sure how you would solve this without a wrapper?

Right.. is this much used? We should brainstorm a bit on this 🙌

Spacing as responsibility of parent element
This will unfortunatley not work in some contexts, where users will need to add it manually themselves. I suggest having a parent element as an alternative, but not the only option.

Could you elaborate on what contexts? :)

@Barsnes
Copy link
Member

Barsnes commented Aug 20, 2024

  • Not necessarily - .ds-btn > svg:first-child is a pretty safe selector? :)

We would in that case need to add

.ds-button:not(.ds-button-only-icon) svg:first-child { ... }
.ds-button:not(.ds-button-only-icon) svg:last-child { ... }

Luckly I don't think there are other elments this applies to, but that's the thing, do we 100% know that we have crossed all the boxes with this? 🤔

Right.. is this much used? We should brainstorm a bit on this 🙌

I believe this is used yes, but I also believe this comes from the old design system it was built on.

Could you elaborate on what contexts? :)

To word myself a bit differently, why should we lock someone into always having to use a wrapper around their text content, when the components themselves could solve it? Why would I need a wrapper if I just have a section with a heading and a paragraph, which would not need a wrapper to begin with.
Having it as an alternative is nice, but the only solution seems like an oversight.

@eirikbacker
Copy link
Contributor Author

@eirikbacker
Copy link
Contributor Author

PS: looks like the danish designsystem does not require a wrapper, unless using prefix/suffix: https://designsystem.dk/komponenter/inputfelter/#prefix-og-suffix-kode

@Barsnes
Copy link
Member

Barsnes commented Aug 20, 2024

Reading thourhg some of these, I would suggest dropping the spacing prop, and not giving them a "stack" component either - as suggested in #2258.
Layout is specific to each website and app, and if we make the component customizable with props such as gap, they might as well create a component that suits them.

@eirikbacker
Copy link
Contributor Author

eirikbacker commented Aug 20, 2024

Agreed 🙌 Stacking/gap is regardless maybe more domain specific, and quite easy to set up. If we want to provide a out-of-the-box-solution for this, we can rather launch this later ☺️

@Barsnes
Copy link
Member

Barsnes commented Aug 20, 2024

Agreed 🙌 Stacking/gap is regardless maybe more domain specific, and quite easy to set up. If we want to provide a out-of-the-box-solution for this, we can rather launch this later ☺️

I would rather suggest easy to copy examples as the way to go :) Gives us less to worry about, but we still give a solution to consumers

@eirikbacker
Copy link
Contributor Author

Okay, so Radix, Tailwind, Chakra, Carbon, Shadcn, Ant and MUI all uses flex or inline-flex on buttons, so I guess we can say for sure this is a established convention 😅 Lets document that our consumers should wrap content in a <span> if they need several text formattings in a button / chip etc, but there is no need for extra HTML elements when using plain text :)

@mimarz
Copy link
Collaborator

mimarz commented Sep 4, 2024

Updated with tasklist for todo components

eirikbacker added a commit that referenced this issue Sep 13, 2024
- Part of #2295 
- Wraps pagination for now, but should be looked more into #2396
- Skipping #2283 for now to avoid too large PR
- Renames `Pagination.Content` to `Pagination.List` following #2221
- Fixes so `Button` with `hidden` attribute is actually hidden ☺️
This was referenced Sep 16, 2024
Barsnes added a commit that referenced this issue Sep 16, 2024
part of #2295

---------

Co-authored-by: Eirik Backer <[email protected]>
eirikbacker added a commit that referenced this issue Sep 16, 2024
- Fixes #2100 🥳 
- Deprecates #2176, #2190
- Built on top of `<u-details>` for [better accessibility on
mobile](https://u-elements.github.io/u-elements/elements/u-details#accessibility)
- Removes `level` from `AccordionHeader` as this is not supported by
native `<details>`
- Removes `onHeaderClick` from `AccordionHeader` as this is identical to
adding a `onClick` handler
- JS-based animation can be removed and replaced by CSS when
`calc-size(auto)` is fully supported 🚀
- Fixes shrinking chevron on mobile/zoom
- Fixes text-align in AccordionHeader on mobile/zoom
- Follows: #2295 
- Also works in `dir="rtl"`

Question: It is now implemented so search-in-page only works when using
`defaultOpen`, as a controlled `open` should not be affected by user
interaction. Just checking - does this make sense to you guys as well?
:)
Barsnes added a commit that referenced this issue Sep 17, 2024
part of #2295
I am not renaming anything is this PR. That belongs in it's own PR
Barsnes added a commit that referenced this issue Sep 17, 2024
eirikbacker added a commit that referenced this issue Sep 17, 2024
eirikbacker added a commit that referenced this issue Sep 17, 2024
- Part of #2295

---------

Co-authored-by: Tobias Barsnes <[email protected]>
eirikbacker added a commit that referenced this issue Sep 18, 2024
Part of #2295 and
#2221
Moves back link into `nav` as this make sense also from a11y perspective
after discussion with NRK

---------

Co-authored-by: Tobias Barsnes <[email protected]>
@eirikbacker eirikbacker removed their assignment Sep 18, 2024
eirikbacker added a commit that referenced this issue Sep 18, 2024
eirikbacker added a commit that referenced this issue Sep 20, 2024
Part of #2295

---------

Co-authored-by: Tobias Barsnes <[email protected]>
eirikbacker added a commit that referenced this issue Sep 23, 2024
- Part of #2295
- Awaiting doing `Paragraph` as we should convert to using `@composes`
in other componentsbefore using data-attributes on paragraph
@Barsnes
Copy link
Member

Barsnes commented Sep 27, 2024

Closing this as completed.
Components have been updated.

@Barsnes Barsnes closed this as completed Sep 27, 2024
@github-project-automation github-project-automation bot moved this from ☂ Epics to ✅ Done in Team Design System Sep 27, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
css @digdir/designsystemet-css ☂️ epic Issues ready
Projects
Status: ✅ Done
Development

No branches or pull requests

4 participants