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

feat(tag): expressive theme update #4166

Merged

Conversation

IgnacioBecerra
Copy link
Contributor

@IgnacioBecerra IgnacioBecerra commented Oct 7, 2020

Related Ticket(s)

#3669

Description

Changing the Tag styles used for Expressive Theme. Bigger font, darker colors, more padding is included.

image

(Allow icon will be included in a separate PR to the carbon-components repository)

Changelog

Changed

  • Expressive theme styles added
  • Increased overall padding
  • Darkened text color
  • Increased line height and font

@IgnacioBecerra IgnacioBecerra added package: styles Work necessary for the Carbon for IBM.com styles package hacktoberfest-accepted labels Oct 7, 2020
@ibmdotcom-bot
Copy link
Contributor

ibmdotcom-bot commented Oct 7, 2020

@ibmdotcom-bot
Copy link
Contributor

ibmdotcom-bot commented Oct 7, 2020

@ibmdotcom-bot
Copy link
Contributor

ibmdotcom-bot commented Oct 7, 2020

Comment on lines 25 to 72
.#{$prefix}--tag--blue {
color: $carbon--blue-80;
}

.#{$prefix}--tag--red {
color: $carbon--red-80;
}

.#{$prefix}--tag--magenta {
color: $carbon--magenta-80;
}

.#{$prefix}--tag--purple {
color: $carbon--purple-80;
}

.#{$prefix}--tag--cyan {
color: $carbon--cyan-80;
}

.#{$prefix}--tag--teal {
color: $carbon--teal-80;
}

.#{$prefix}--tag--green {
color: $carbon--green-80;
}

.#{$prefix}--tag--gray {
color: $carbon--gray-80;
}

.#{$prefix}--tag--cool-gray {
color: $carbon--cool-gray-80;
}

.#{$prefix}--tag--warm-gray {
color: $carbon--warm-gray-80;
}

.#{$prefix}--tag--filter {
padding-right: $spacing-04;
}

.#{$prefix}--tag--disabled,
.#{$prefix}--tag--filter.#{$prefix}--tag--disabled {
color: $disabled-02;
}
Copy link
Contributor

Choose a reason for hiding this comment

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

Wondering the color styles in here were actually needed, as they are implemented in Carbon core.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The main difference between these and those is that the colors here are 80 grade while the others are 70 grade. One of the new things in the Expressive theme is the darkening of the colors. But if there's another way to get 80 grade colors from the Carbon core classes that I'm missing, please let me know!

Copy link
Contributor

Choose a reason for hiding this comment

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

One of the new things in the Expressive theme is the darkening of the colors.

Thank you for pointing it out @IgnacioBecerra! Sounds that we need more system-wide change, similar to what we do for type tokens. I think we can start with changing color tokens at https://github.com/carbon-design-system/ibm-dotcom-library/blob/v1.11.0/packages/styles/scss/themes/expressive/_tokens.scss#L371-L479.

@jeffchew Is this something worthwhile as a engineering huddle topic?

Copy link
Member

Choose a reason for hiding this comment

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

Sorry, I missed this comment. Let's bring up as a huddle topic @IgnacioBecerra @asudoh .

Copy link
Contributor

Choose a reason for hiding this comment

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

Cool, done!

@IgnacioBecerra IgnacioBecerra marked this pull request as ready for review October 13, 2020 01:39
@@ -16,6 +16,74 @@
/// @group tag-expressive
@mixin tag-expressive {
.#{$prefix}--tag {
@include carbon--type-style('body-long-01');
Copy link
Contributor

Choose a reason for hiding this comment

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

I don't think we want to change the type token to body-long-01 here because it's increasing the text size to 16px?

I thought the expressive theme token updates should have updated the original token label-01 to 14px per the design specs, unless I am mistaken.

@jeffchew
Copy link
Member

Recapping some of the bullets from conversations:

  • update the height in the expressive theme
  • contribute color changes through Carbon repo so the system stays aligned between Carbon productive/expressive
  • add the variation for icon before the tag / contribute back

Based on this, we will not be handling the color changes in this PR. @IgnacioBecerra can you update?

@oliviaflory @wonilsuhibm you mentioned also the variation for icon you still want in but we eventually want to contribute back, right?

cc: @asudoh

Copy link
Contributor

@asudoh asudoh left a comment

Choose a reason for hiding this comment

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

Thanks @IgnacioBecerra for the updates!

@@ -16,6 +16,36 @@
/// @group tag-expressive
@mixin tag-expressive {
.#{$prefix}--tag {
@include carbon--type-style('label-01');

line-height: 20;
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
line-height: 20;
line-height: carbon--rem(20px);

Comment on lines 40 to 41
width: 1.25rem;
height: 1.25rem;
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
width: 1.25rem;
height: 1.25rem;
width: $icon-size-01;
height: $icon-size-01;

@oliviaflory
Copy link
Contributor

@IgnacioBecerra This might be nit picky, but is there a way to change the default label in storybook? It's a bit weird that it says "This is not a tag" when it is a tag..

Screen Shot 2020-10-22 at 12 13 46 PM

Ideally it would say "This is a tag"

Realize this might not be in scope, so if it's a difficult thing I can open another issue to address this.

@IgnacioBecerra
Copy link
Contributor Author

@asudoh I had used carbon--rem(20px) before, but eslint doesn't expect the line-height value to have px (weird), so left it at 20. I changed it to carbon--rem(20) since it still appears to work.

@oliviaflory I always thought that was a weird label to have when I was testing, and even the carbon-components Storybook has the regular This is a tag. Changed it to that in the latest commit!

@asudoh
Copy link
Contributor

asudoh commented Oct 22, 2020

@IgnacioBecerra What specific StyleLint error did you get?

@IgnacioBecerra
Copy link
Contributor Author

@asudoh
The error I got was: unexpected unit px for property line-height

Screen Shot 2020-10-22 at 9 37 01 AM

When resolving this issue I found the docs, which said that having line-height without px wasn't considered a violation.

@asudoh
Copy link
Contributor

asudoh commented Oct 22, 2020

I see, it's hostile to how Carbon core defines one-height`. Also unitless is a different animal from pixel unit. Given those, we have to change the StyleLint rule. I can take a look at it tomorrow.

@IgnacioBecerra
Copy link
Contributor Author

I see, that's good to know unitless is not the same as pixel unit. Thanks Akira!

@asudoh
Copy link
Contributor

asudoh commented Oct 22, 2020

https://github.com/carbon-design-system/carbon-for-ibm-dotcom/blob/v1.12.0/packages/stylelint-config-ibmdotcom/rules/limit-language-features.js#L80 seems the culprit. A couple of alternatives are:

  • Remove the rule
  • Disable the rule for the particular line (Given carbon--rem() makes sure we use the rem unit)

@IgnacioBecerra
Copy link
Contributor Author

@asudoh Ended up disabling the rule for that specific line. 👍

Copy link
Contributor

@asudoh asudoh left a comment

Choose a reason for hiding this comment

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

Cool, just a suggestion to make the ignore thing more specific.

Copy link
Contributor

@asudoh asudoh left a comment

Choose a reason for hiding this comment

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

Thanks @IgnacioBecerra for all the updates!

@jeffchew jeffchew added the Ready to merge Label for the pull requests that are ready to merge label Oct 26, 2020
@kodiakhq kodiakhq bot merged commit 73e13a0 into carbon-design-system:master Oct 26, 2020
ariellalgilmore pushed a commit to ariellalgilmore/carbon-for-ibm-dotcom that referenced this pull request Oct 26, 2020
### Related Ticket(s)

carbon-design-system#3669

### Description

Changing the Tag styles used for Expressive Theme. Bigger font, darker colors, more padding is included. 

![image](https://user-images.githubusercontent.com/24970122/95804112-a5ca1e80-0cb6-11eb-90e3-9bea60302f0a.png)

(Allow icon will be included in a separate PR to the carbon-components repository)

### Changelog

**Changed**

- Expressive theme styles added
- Increased overall padding
- Darkened text color
- Increased line height and font


<!-- React and Web Component deploy previews are enabled by default. -->
<!-- To enable additional available deploy previews, apply the following -->
<!-- labels for the corresponding package: -->
<!-- *** "package: vanilla": Vanilla -->
<!-- *** "package: services": Services -->
<!-- *** "package: utilities": Utilities -->
<!-- *** "package: styles": Carbon Expressive, React (Expressive) -->
<!-- *** "RTL": React (RTL) -->
<!-- *** "feature flag": React (experimental) -->
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
package: styles Work necessary for the Carbon for IBM.com styles package Ready to merge Label for the pull requests that are ready to merge
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants