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

Themes: Add updated Indigo themes for Slider, Text Area, and Tree #1134

Merged
merged 17 commits into from
May 15, 2024

Conversation

simeonoff
Copy link
Collaborator

@simeonoff simeonoff commented Apr 5, 2024

  • Slider
  • Text Area
  • Tree

@simeonoff simeonoff changed the title Themes: Update Indigo Component Themes Themes: Add updated Indigo component themes Apr 11, 2024
SisIvanova
SisIvanova previously approved these changes Apr 11, 2024
SisIvanova
SisIvanova previously approved these changes Apr 16, 2024
@simeonoff simeonoff changed the title Themes: Add updated Indigo component themes Themes: Add updated Indigo themes for Slider, Text Area, and Tree May 13, 2024
@simeonoff simeonoff added the tree label May 13, 2024
@simeonoff simeonoff merged commit a7a8b0a into master May 15, 2024
5 checks passed
@simeonoff simeonoff deleted the simeonoff/indigo-themes branch May 15, 2024 08:05
@AnjiManova
Copy link

AnjiManova commented May 15, 2024

Text Area

  • Based on the Input component in the Text Area in the Invalid state, the helper text (hint) shouldn't have color coding. Applicable for Light and Dark mode and Warning and Correct state as well.
image

I have something else, but I'm not sure if it's relevant, so if it's not, ignore my comment. The drag indicator color does not change between the enabled and disabled states, but as far as I see, this is valid for all themes.

There is an additional comment related to the Text Area behaviour of saving the filled text and switching to Disabled, Read-only, Invalid, and so on, but we will create a separate issue for this, as the front-end developers recommended.

I saw nothing else as a discrepancy between the UI kit and the implementation.

@sbayreva
Copy link

sbayreva commented May 15, 2024

Slider

  1. The track should have 4px radius
  2. The tickmarks should start right under the end of the track.
Screenshot 2024-05-15 at 14 29 34

3.Тhe "Discrete Regions" should start from 0 as in the Handoff

Screenshot 2024-05-15 at 17 11 34
  1. The focus border of the thumb should be 3px

  2. Тhe triangle of the label tooltip should have 12px height
    @andiesm813 please, check the comments and also, please check the behaviour of the Hover and Focus states - whether each element should have these states separately or when on Hover or Focused the whole component should be in that state.

@imincheva
Copy link

imincheva commented May 15, 2024

Tree:

  • The left and right paddings should be fixed to 16px, 12px and 8px for large, medium and small respectively.
  • The indentation should be 24px for all display densities.
  • Might be irrelevant, but in the UI kits we have a specific stylisation for the hyperlinks, which I believe isn't implemented. Same for the custom chevron, don't know what the situation is with that.
  • Тhis is probably some bug, but when I hover the first item, it doesn't inherit the correct color for the foreground in both light and dark mode. In the rest of the items this problem isn't apparent.

Edit: There are two things, which need to be applied in Angular as well:

  • When a node is selected, it inherits a wrong color (--foreground-active). It should be fixed to (--foreground)
  • The indentation should be 24px for all display densities.

@andiesm813
Copy link

Slider

  1. The track should have 4px radius
  2. The tickmarks should start right under the end of the track.
Screenshot 2024-05-15 at 14 29 34 3.Тhe "Discrete Regions" should start from 0 as in the Handoff Screenshot 2024-05-15 at 17 11 34 4. The focus border of the thumb should be 3px 5. Тhe triangle of the label tooltip should have 12px height @andiesm813 please, check the comments and also, please check the behaviour of the Hover and Focus states - whether each element should have these states separately or when on Hover or Focused the whole component should be in that state.

@sbayreva
All your comments (1-4) are good. About #1 (radius of the track), it was also not implemented in Angular (from what i'm seeing).

About the hover and focus states, the current behavior in WC is the same as in the Angular component. Track and track base do hover together, but the thumb hovers separately, only when hovered on the thumb. So IMO it's all good as implemented now.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

9 participants