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

[Status indicators] Implement Icon indicator #17539

Open
Tracked by #13429
thyhmdo opened this issue Sep 20, 2024 · 4 comments · May be fixed by #18191
Open
Tracked by #13429

[Status indicators] Implement Icon indicator #17539

thyhmdo opened this issue Sep 20, 2024 · 4 comments · May be fixed by #18191
Assignees
Milestone

Comments

@thyhmdo
Copy link
Member

thyhmdo commented Sep 20, 2024


Specs + Storybook implementation

Final icon specs

@sstrubberg
Copy link
Member

Devs need to meet and create a strategy on how to go about implementing this.

@tay1orjones
Copy link
Member

tay1orjones commented Oct 17, 2024

Tokens

  • make the new status tokens
  • make the new a11y background token

Requirements

  • Layer support? or is it just light and dark? Doesn't matter, it'll use the text tokens for a11y color on the label
  • It should support the contextual layout tokens for size control
  • Use the icons from carbon icons
  • Use type tokens for text
<IconIndicator kind="error" label="my label" size={20}>

Props

  • kind, required enum, defaults to ???, [error, caution-major (to match what we do in notification), etc.]
  • label, required, for a11y, no default
  • hideLabel, boolean, if true throws the label into a Tooltip Tooltip will not be implemented
  • size, [16, 20] so it matches icon's size prop, not required so it will work with Layout
  • doesn't accept children, self-closing tag

Styles

  • Vertical alignment icon and text
  • Margin between the icon and the text
  • Apply the relevant tokens, etc. per classname

Storybook

  • Will have it's own section
    • default story showcases all of them with the sample labels from the spec "Caution major" etc.
    • playground, a single one with full controls enabled for all props
  • Add it to a DataTable story? Dynamic?
  • Add it to a Tile story?

Open questions

  • When will $status-accessibility-background be used? In what components?
  • Would caution minor/major need a different token value for $status-accessibility-background?
  • Why do some status-blue and status-gray ones not use the $status-accessibility-background
  • Tooltip
    • If hideLabel, tooltip needs a trigger, which needs to be interactive, but there's no interaction, it's just for the tooltip. Is that okay a11y-wise? Or what would be a better approach?
    • This will mean that text/label is not optional
    • ~Does it need to be a ToggleTip? ~
      • ~Surely the first ask we'll get for this is to have a link inside the tooltip to direct the user to more information somewhere or an action to take. ~
      • If we either need a node prop for the content or revise the whole API to be more composable and accept children.
  • Should this support the density contextual layout tokens and shrink the margin between the two from 8px to 4px? Or always 8px?
    A: No density for now. Size should be supported though

@tay1orjones tay1orjones moved this from 🪆 Needs Refined to ⏱ Backlog in Design System Oct 17, 2024
@tay1orjones
Copy link
Member

tay1orjones commented Oct 17, 2024

Open questions

  • When will $status-accessibility-background be used? In what components?
  • Warning status has a dark background behind it, which kinda looks like the fill of some icons
  • In the white theme, we need another $status-accessibility-background-inverted to cover the caution icons that need a gray 100 "fill"
  • Would caution minor/major need a different token value for $status-accessibility-background?
  • Why do some status-blue and status-gray ones not use the $status-accessibility-background
  • For the ones without, they should have a transparent background for the non-color area
  • Does this mean the transparent ones need values for the tokens on each layer to maintain a11y contrast.
  • We can set the background for all of them if we need to
  • Thy will get back to us on this aspect
  • Tooltip

    • If hideLabel, tooltip needs a trigger, which needs to be interactive, but there's no interaction, it's just for the tooltip. Is that okay a11y-wise? Or what would be a better approach?

    • This will mean that text/label is not optional

    • Does it need to be a ToggleTip?

      • Surely the first ask we'll get for this is to have a link inside the tooltip to direct the user to more information somewhere or an action to take.
      • If we either need a node prop for the content or revise the whole API to be more composable and accept children.
  • We're going to totally remove hideLabel, no tooltip support, the label will always be visible
  • Should this support the density contextual layout tokens and shrink the margin between the two from 8px to 4px? Or always 8px?

In storybook, just do datatable for now.

@tay1orjones
Copy link
Member

Discussed today, we could start with react and styles, then branch off that to do web components. 2PRs total

@ariellalgilmore ariellalgilmore linked a pull request Dec 3, 2024 that will close this issue
@2nikhiltom 2nikhiltom moved this from 🏗 In Progress to 🚦 In Review in Design System Dec 4, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Status: 🚦 In Review
Development

Successfully merging a pull request may close this issue.

6 participants