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

Indigo Themes: Update List, Progress, and Dropdown #1224

Merged
merged 20 commits into from
Jun 12, 2024

Conversation

simeonoff
Copy link
Collaborator

No description provided.

@sbayreva
Copy link

sbayreva commented Jun 3, 2024

Linear bar

  1. The space between the label and the track seems to be bigger than 4px even though it says it is. The size of the whole component should be 24px as in Angular, here it is 28px:
Screenshot 2024-06-03 at 10 20 09

Circular progress

  1. I am not sure whether Subtitle 2 it is used as a style for the label
  2. The size of the component should be 48px:
Screenshot 2024-06-03 at 10 31 54 Screenshot 2024-06-03 at 10 33 19

@imincheva

This comment was marked as resolved.

@andiesm813
Copy link

Linear bar

  1. The space between the label and the track seems to be bigger than 4px even though it says it is. The size of the whole component should be 24px as in Angular, here it is 28px:
Screenshot 2024-06-03 at 10 20 09 **Circular progress**
  1. I am not sure whether Subtitle 2 it is used as a style for the label
  2. The size of the component should be 48px:

Screenshot 2024-06-03 at 10 31 54 Screenshot 2024-06-03 at 10 33 19

Agree to all comments.
Apparently it is Subtitle 2 @sbayreva .... if it wasnt, it would look WAAAY lighter! see below! So this is not an issue. It's good! ✅

image

@andiesm813
Copy link

List

Agree to all comments made by @imincheva ... and i'm adding the same comments i added for Angular, which apply here as well.

LIGHT THEME

--header-text-color has the correct value in the CSS, however, in the samples, the header seems way darker than grays600. It looks almost as dark as grays900.

SIZES
The implemented sizes are incorrect. All the sizes that are implemented for COMFORTABLE correspond to COSY. And the ones implemented for COSY correspond to COMPACT. So the sizes for LARGE need to be defined by me. See below.
These sizes specified below are consistent with the sizes of the TREE component.

Comfy - Large

  • The height of the header container should be 32px.
  • The height of the single line item should be 32px.
  • List Item inner paddings: 8px 16px.

Cosy - Medium

  • The height of the header container should be 28px.
  • The height of the single line item should be 28px.
  • List Item inner paddings: 6px 12px.

Compact - Small

  • The height of the header container should be 24px.
  • The height of the single line item should be 24px.
  • List Item inner paddings: 4px 8px.

ADDITIONAL COMMENT
There is no ripple effect on click here, on click. This is the expected behavior. Angular should match this too.

@AnjiManova
Copy link

AnjiManova commented Jun 6, 2024

Dropdown

Currently the default size is Large, I think it should be Medium, but @andiesm813 should confirm.

If this is irrelevant, ignore it: the focused state in Angular is triggered with igx-drop-down__item--focused, while in the WebC is Active and the border uses --focused-item-border-color. I think we talked about this at some point that this is native behaviour and focused and active are the same but ...

  • separate issue for the borders roundness or whatever the problem is in dark mode because it is related to all kits

@andiesm813
Copy link

@AnjiManova yes, the default should be medium. I see that it is medium now... maybe it was updated since your comment....

@SisIvanova
Copy link
Contributor

Linear bar

  1. The space between the label and the track seems to be bigger than 4px even though it says it is. The size of the whole component should be 24px as in Angular, here it is 28px:
Screenshot 2024-06-03 at 10 20 09 **Circular progress**
  1. I am not sure whether Subtitle 2 it is used as a style for the label
  2. The size of the component should be 48px:

Screenshot 2024-06-03 at 10 31 54 Screenshot 2024-06-03 at 10 33 19

Agree to all comments. Apparently it is Subtitle 2 @sbayreva .... if it wasnt, it would look WAAAY lighter! see below! So this is not an issue. It's good! ✅
image

  • Point 1 is verified
  • Point 2 is verified
  • About the font style Subtitle 2, I am not sure whether the font is used as a style, that's why I added the comment. @SisIvanova can you please, confirm whether font style Subtitle 2 is used for the labels of the Linear and Circular Progress Indicators?

@sbayreva As Andie said it is Subtitle 2. You can also see it in the samples applied on the part label/value.
Screenshot 2024-06-10 at 9 44 39 AM

@SisIvanova
Copy link
Contributor

SisIvanova commented Jun 10, 2024

@AnjiManova The dropdown default size should be fixed.

@andiesm813
Copy link

Linear bar

  1. The space between the label and the track seems to be bigger than 4px even though it says it is. The size of the whole component should be 24px as in Angular, here it is 28px:
Screenshot 2024-06-03 at 10 20 09 **Circular progress**
  1. I am not sure whether Subtitle 2 it is used as a style for the label
  2. The size of the component should be 48px:

Screenshot 2024-06-03 at 10 31 54 Screenshot 2024-06-03 at 10 33 19

Agree to all comments. Apparently it is Subtitle 2 @sbayreva .... if it wasnt, it would look WAAAY lighter! see below! So this is not an issue. It's good! ✅
image

  • Point 1 is verified
  • Point 2 is verified
  • About the font style Subtitle 2, I am not sure whether the font is used as a style, that's why I added the comment. @SisIvanova can you please, confirm whether font style Subtitle 2 is used for the labels of the Linear and Circular Progress Indicators?

@sbayreva As Andie said it is Subtitle 2. You can also see it in the samples applied on the part label/value. Screenshot 2024-06-10 at 9 44 39 AM

So PROGRESS looks all good then ✅

@andiesm813
Copy link

@AnjiManova The dropdown default size should be fixed.

Looks fixed to me! thanks for the update! ✅

@andiesm813
Copy link

LIST

Some ofl the fixes requested for this component have not been applied yet, if i'm not mistaken. Maybe it's still in progress?

Pending fixes

  1. The height of the header text is still not fixed to 16px.
  2. The paddings of the list headers are only fixed for large size. The others remain incorrect. In my previous comments were the correct paddings (list item inner paddings) and expected header sizes, if all is correct (header text fixed height affects the total height of the header).
  3. Row-gap on the list item (space between start, content, end) should be 8px, not 16px
  4. List Item small inner padding is still not correct. It should be 4px 8px, and it's currently 2px 4px.
  5. There is a gap between title and subtitle, but i'm not sure if it's 2px or more.
  6. The title should be set to body 2 as well as the subtitle. The only difference between title and subtitle is the foreground color.
  7. Gap between secondary actions should be 0.5rem (currently it's 0.375rem)
  8. Also not sure which List size is the default size. Currently it's a mixture of Large and Medium. Medium should be the default size.

image

@andiesm813
Copy link

LIST

Some ofl the fixes requested for this component have not been applied yet, if i'm not mistaken. Maybe it's still in progress?

Pending fixes

  1. The height of the header text is still not fixed to 16px.
  2. The paddings of the list headers are only fixed for large size. The others remain incorrect. In my previous comments were the correct paddings (list item inner paddings) and expected header sizes, if all is correct (header text fixed height affects the total height of the header).
  3. Row-gap on the list item (space between start, content, end) should be 8px, not 16px
  4. List Item small inner padding is still not correct. It should be 4px 8px, and it's currently 2px 4px.
  5. There is a gap between title and subtitle, but i'm not sure if it's 2px or more.
  6. The title should be set to body 2 as well as the subtitle. The only difference between title and subtitle is the foreground color.
  7. Gap between secondary actions should be 0.5rem (currently it's 0.375rem)
  8. Also not sure which List size is the default size. Currently it's a mixture of Large and Medium. Medium should be the default size.

image

Points 1-8 have been verified. So i can say LIST is all good ✅

[part='title'] {
@include type-style('body-1');
}

[part='subtitle'] {
@include type-style('body-2');
}

[name='start']::slotted(*) {
Copy link
Contributor

Choose a reason for hiding this comment

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

@desig9stein You don't need these margins here. You already have them in the list-item.common

@@ -14,3 +14,11 @@ $theme: $fluent;
[part='title'] {
@include type-style('caption');
}

[name='start']::slotted(*) {
Copy link
Contributor

Choose a reason for hiding this comment

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

@desig9stein Same as in bootstrap.

@SisIvanova SisIvanova self-requested a review June 12, 2024 08:28
SisIvanova
SisIvanova previously approved these changes Jun 12, 2024
@simeonoff simeonoff merged commit 7a956b3 into master Jun 12, 2024
5 checks passed
@simeonoff simeonoff deleted the simeonoff/indigo-themes branch June 12, 2024 11:33
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

7 participants