feat: add Label references to utilities and docs#3621
Conversation
github-project-automation
Bot
moved this to In Progress / Draft
in 🟣 [Orange-Boosted-Bootstrap] PRs Board
boosted-bot
moved this from In Progress / Draft
to Need Dev Review
in 🟣 [Orange-Boosted-Bootstrap] PRs Board
✅ Deploy Preview for boosted ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
louismaximepiton
left a comment
louismaximepiton
left a comment
There was a problem hiding this comment.
I think the changes in */_semantics.scss files aren't needed since the variables are already defined in the files. In general, these files shouldn't be changed at all, but with tokenator automated PRs.
.bundlewatch.json needs to be updated as well for the CI to pass correctly.
boosted-bot
moved this from Need Dev Review
to Dev Review In Progress
in 🟣 [Orange-Boosted-Bootstrap] PRs Board
…pdated changelog with labels utilities
boosted-bot
moved this from Dev Review In Progress
to Need Lead Dev Review
in 🟣 [Orange-Boosted-Bootstrap] PRs Board
boosted-bot
moved this from Need Lead Dev Review
to Dev Review In Progress
in 🟣 [Orange-Boosted-Bootstrap] PRs Board
MaxLardenois
left a comment
MaxLardenois
left a comment
There was a problem hiding this comment.
A little suggestion here
louismaximepiton
left a comment
louismaximepiton
left a comment
There was a problem hiding this comment.
There are some mixin that can be replaced such as in alerts where we define the max-width and the get font size, don't know if there are more
| @mixin get-font-size($font-size-ref: "display-large") { | ||
| @if str-slice($font-size-ref, 1, 5) != "label" and str-slice($font-size-ref, 1, 4) != "code" { | ||
| max-width: var(--#{$prefix}font-max-width-#{$font-size-ref}); | ||
| @mixin get-font-size($font-size-ref: "display-large", $label-with-max-width: false) { |
There was a problem hiding this comment.
Not a big fan of getting only label with the parameter, but le'ts find out later !
There was a problem hiding this comment.
In fact, I think I'd go with max-width by default and call when I don't need max-width
There was a problem hiding this comment.
Not sure I understand your points
Maybe we should determine if the max width should be default or not on labels
There was a problem hiding this comment.
My point here is to rather have get-font-size($font-size-ref: "display-large", $apply-max-width: true) and change the calls wherever not applied. I'll try something this afternoon to see the changes in one commit, then we'll decide
There was a problem hiding this comment.
Tried something, tell me i it should b reverted or not
There was a problem hiding this comment.
Ok so I thought about it and imo we should not do this, at least in this PR, because:
- it introduces an (albeit small) breaking change not absolutely necessary to close the issue
- it necessitates to change a lot of files
- the API is a little deceptive (
@include get-font-size('code-something', true)do not add the max-width even though the parameter looks like it would work
I think it would be worthwhile to merge this as it was into v1.4 and create another issue/PR with your commit to explore a future API change on get-font-size.
github-project-automation
Bot
moved this from Dev Review In Progress
to Done
in 🟣 [Orange-Boosted-Bootstrap] PRs Board
Types of change
Related issues
Closes #3610
Context & Motivation
Description
Add label-{small|medium|large|xlarge} references to classes fs-* and to Typography and Text docs.
Checklists
Checklist (for Core Team only)
Progression (for Core Team only)
ouds/mainfollowing conventional commitLive previews