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

Update design-tokens & build script to support new border-radius tokens #2476

Closed
Tracked by #2287
mimarz opened this issue Sep 20, 2024 · 9 comments · Fixed by #2497
Closed
Tracked by #2287

Update design-tokens & build script to support new border-radius tokens #2476

mimarz opened this issue Sep 20, 2024 · 9 comments · Fixed by #2497
Assignees
Labels
cli @digdir/designsystemet $ tokens Everything related to tokens and Work related to @digdir/designsystemet-theme

Comments

@mimarz
Copy link
Collaborator

mimarz commented Sep 20, 2024

No description provided.

@mimarz mimarz added $ tokens Everything related to tokens and Work related to @digdir/designsystemet-theme cli @digdir/designsystemet labels Sep 20, 2024
@mimarz mimarz moved this from 🔵 Inbox to 🏗 In progress in Team Design System Sep 20, 2024
@mimarz mimarz changed the title Update design-tokens & build script Update design-tokens & build script to support new border-radius tokens Sep 24, 2024
@Febakke
Copy link
Member

Febakke commented Oct 7, 2024

New Tokens on components

Small

  • Avatar (Square)
  • Checkbox
  • Tag

Medium

  • Alert
  • Accordion
  • Dropdown
  • Popover
  • Text area
  • Chip with checkbox
  • Textfield
  • Select
  • Combobox
    • Hover on elements inside list has small
  • Table with border

Large

  • Card
  • Error summary
  • Sceleton (rectangle)
  • Modal

Default

  • Button
  • Search
  • Tooltip
  • Textfield

Full

  • Avatar (Circle)
  • Badge
  • Chip
  • Helptext ("?" circle)
  • Radio (circle)
  • Switch
  • Skeleton circle and text

@Febakke
Copy link
Member

Febakke commented Oct 9, 2024

After meeting with @Thunear

Moved the default scale down one step
medium 12 -> 8
large 24-> 20

Text-field now use medium instead of default

@unekinn
Copy link
Contributor

unekinn commented Oct 10, 2024

@Febakke some questions:

Components not mentioned in the comment above

Should these be unchanged?

  • Chip with checkbox: large
  • Helptext: full
  • Radio: hardcoded to 50%
  • Select: medium
  • Switch: full
  • Table with border: medium
  • Tabs: full
  • Tag: small
  • Toggle group: medium
  • Tooltip: default

Other questions

  • Shouldn't all form elements have the same radius? Now Combobox had default radius, while Select, Textfield and TextArea have medium
  • Search and Textfield have different radius, is this intentional?

@unekinn
Copy link
Contributor

unekinn commented Oct 10, 2024

Fairly certain "Chip with checkbox" needs to have medium radius for the chip and small radius for the checkbox, so I'm implementing that.

@Febakke
Copy link
Member

Febakke commented Oct 15, 2024

@unekinn
I updated the comment over here with the components that where missing. But ill have a summary here as well

Some comments

  • Chip with checkbox: I agree, it should have medium border-radius as you pointed out
  • Helptext, I believe you are talking about the iconbutton? Yes it should be full
  • Radio, Im not sure what 50% means, but it should always be a circle and have full border radius
  • Select should have medium
    • After a discussion with @Thunear we agreed that textfield should not become round as I first wrote. The reason is that it will not look right in form context. (How it aligns with the label) I think all the "form" fields should have medium border-radius. Search is a different story and can and often has a circular look. So it should use default.
  • Switch, Always full
  • Table with border. If there are noe border on the table there is no point in having a border-radius. In Figma this is something you have to set up yourself. But if we need a default value i would use medium
  • Tabs, there are no border around tabs. Where is this applied?
  • Tag should always keep their square shape. small
  • Toggle group, Ill write more about this further down 👇 But I think toggle group can be full
  • Tooltip can be circular. full

Logic for nicer components

I have seen an issue with components that has elements inside other elements where radius is applied to both. When design components like this you often apply more radius to the outer element. Lets use toggle groups as an example.

Skjermbilde 2024-10-15 kl  15 04 31

Here both buttonand the togglegroup border use 8 border-radius. It looks kind of weird.

Skjermbilde 2024-10-15 kl  15 06 07

In this example we use 8 for button and 12 for the outside border. If you add the distance from the inner element to the border of the outside and add them together you get the correct border-radius. I havent though of this issue before and this is not the worst example. This will also cause "design" issues with our focus indicator.

The best case would be to handle this with tokens in some way. That means we could replicate correctly in Figma, but i fear this might be to complex.

@unekinn
Copy link
Contributor

unekinn commented Oct 16, 2024

Tabs, there are no border around tabs. Where is this applied?

The radius is on the line under current tab and hovered tabs. As detailed in Slack this is incorrectly implemented currently, because of a typo(?), so there is currently no rounding in the React component.

Tooltip can be circular. full

Like this?
image

Toggle group, Ill write more about this further down 👇 But I think toggle group can be full

Since your example is using the button (which has default radius) as reference, I assume it should be border-radius.default + spacing.1 and not full?

@unekinn
Copy link
Contributor

unekinn commented Oct 18, 2024

Someone also needs to decide what tokens to use for these values in storefront, as they don't exist after these border-radius changes

Image

@Febakke
Copy link
Member

Febakke commented Oct 21, 2024

Tabs, there are no border around tabs. Where is this applied?

The radius is on the line under current tab and hovered tabs. As detailed in Slack this is incorrectly implemented currently, because of a typo(?), so there is currently no rounding in the React component.

Tooltip can be circular. full

Like this? image

Toggle group, Ill write more about this further down 👇 But I think toggle group can be full

Since your example is using the button (which has default radius) as reference, I assume it should be border-radius.default + spacing.1 and not full?

I think tooltip can be full yes, are there any issues with doing this?

Togglegroup
We might have an issue here, I see three ways the border-radius can go

  1. border-radius.default + spacing.1 In most cases this will be correct
  2. border-radius.default=0 now we have an issue. If border radius is set to 0 on button then it should be 0 on the whole togglegroup
  3. border-radius.default=9999 Is there a problem with border-radius being set to 10 003? 😄

@Febakke
Copy link
Member

Febakke commented Oct 21, 2024

Someone also needs to decide what tokens to use for these values in storefront, as they don't exist after these border-radius changes

Image

Im not sure what elements they are referencing, but I would guess that they should be set to large

@mimarz mimarz closed this as completed in 5f51c95 Oct 22, 2024
@github-project-automation github-project-automation bot moved this from 👀 Ready for review to ✅ Done in Team Design System Oct 22, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cli @digdir/designsystemet $ tokens Everything related to tokens and Work related to @digdir/designsystemet-theme
Projects
Status: ✅ Done
Development

Successfully merging a pull request may close this issue.

3 participants