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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

Update border and font in chips #4850

Merged

Conversation

ClementChaumel
Copy link
Contributor

@ClementChaumel ClementChaumel commented Jul 27, 2023

Done

reduce width of the border

change font weight

use real small caps (instead of small uppercase)

Fixes WD-5198

QA

  • Open demo
  • Check that it looks ok

Check if PR is ready for release

If this PR contains Vanilla SCSS code changes, it should contain the following changes to make sure it's ready for the release:

  • PR should have one of the following labels to automatically categorise it in release notes:
    • Feature 馃巵, Breaking Change 馃挘, Bug 馃悰, Documentation 馃摑, Maintenance 馃敤.
  • Vanilla version in package.json should be updated relative to the most recent release, following semver convention:
    • if CSS class names are not changed it can be bugfix relesase (x.x.X)
    • if CSS class names are changed/added/removed it should be minor version (x.X.0)
    • see the wiki for more details
  • Any changes to component class names (new patterns, variants, removed or added features) should be listed on the what's new page.

Screenshots

image

@webteam-app
Copy link

webteam-app commented Jul 27, 2023

@ClementChaumel ClementChaumel force-pushed the WD-5198-update-border-and-font-in-chips branch from eb68bbe to a47ef16 Compare July 27, 2023 13:26
@ClementChaumel
Copy link
Contributor Author

@bartaz Do you reckon it requires being mentioned in what's new? I feel like it's a small enough change to omit it

@bartaz
Copy link
Contributor

bartaz commented Jul 27, 2023

@bartaz Do you reckon it requires being mentioned in what's new? I feel like it's a small enough change to omit it

No, purely visual changes that don't change the CSS class names are usually not mentined (unless they are really significant), so feel free to skip it here.

@bartaz
Copy link
Contributor

bartaz commented Jul 27, 2023

It seems that chips that have small caps lead are getting larger than the other ones, this should not happen, they should all be same size as before:

image

@ClementChaumel
Copy link
Contributor Author

It seems that chips that have small caps lead are getting larger than the other ones, this should not happen, they should all be same size as before:

image

I'm using the %small-caps-text; placeholder should I also import the small text one?

@bartaz
Copy link
Contributor

bartaz commented Jul 27, 2023

I'm using the %small-caps-text; placeholder should I also import the small text one?

Maybe? There seems to be line height difference, so if chip value uses small size, I guess lead part should as well.
I don't know if placeholders for small caps and small text wont conflict on some styles, but I guess we may need some combination of the two then.

Copy link
Contributor

@bartaz bartaz left a comment

Choose a reason for hiding this comment

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

From Percy it seems like chips got somehow 1px bigger or something, but I think it's OK.

Thanks!

@bartaz bartaz merged commit 0f75a0e into canonical:main Jul 31, 2023
5 checks passed
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

3 participants