feat: Add skeleton component#4512
Conversation
4764d55 to
80f81f0
Compare
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #4512 +/- ##
=======================================
Coverage 97.42% 97.42%
=======================================
Files 936 939 +3
Lines 29626 29645 +19
Branches 10764 10771 +7
=======================================
+ Hits 28864 28883 +19
Misses 755 755
Partials 7 7 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
03067b6 to
10627ed
Compare
10627ed to
b7f0259
Compare
b7f0259 to
1388ca3
Compare
| } | ||
|
|
||
| .variant-text-body-s { | ||
| font-size: awsui.$font-size-body-s; |
There was a problem hiding this comment.
- Are both font size and line height necessary?
- Will this approach reliably give the box the desired size even if empty spaces are stripped out from the HTML output? This might not work if our code gets transformed from
<tag>
<tag/>
</tag>
to
<tag><tag/></tag>
There was a problem hiding this comment.
Yes, both are needed to get the correct size. I've tested with spaces stripped and it seems to also work fine in that scenario.
There was a problem hiding this comment.
OK. Looks like these rule pairs (font size and line height) should almost always go together across the system, plus there is some overlap between this code and the styles defined in src/internal/styles/typography/mixins.scss. What do you tink about moving them there and reuse them?
For example, in src/internal/styles/typography/mixins.scss:
// New mixin
@mixin text-body-s {
font-size: awsui.$font-size-body-s;
line-height: awsui.$line-height-body-s;
}
// New mixin
@mixin .text-body-m {
font-size: awsui.$font-size-body-m;
line-height: awsui.$line-height-body-m;
}
(...)
// Existing mixin, refactored
@mixin font-body-s {
@include text-body-s;
// Some of the existing mixins contain extra rules besides font size and line height
letter-spacing: awsui.$letter-spacing-body-s;
}
// Existing mixin, refactored
@mixin font-body-m {
@include text-body-m;
}
And in this new file:
.variant-text-body-s {
@include text-body-s;
}
.variant-text-body-m {
@include text-body-m;
}
(...)
There was a problem hiding this comment.
looking at font-size-body-s for example, there are more places where it's used without the lineheight than with, so I think there's not much to be gained from putting this into mixins
Description
Related links, issue #, if available: n/a
How has this been tested?
Review checklist
The following items are to be evaluated by the author(s) and the reviewer(s).
Correctness
CONTRIBUTING.md.CONTRIBUTING.md.Security
checkSafeUrlfunction.Testing
By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.