Merged
Conversation
Contributor
|
woohoo — good to see this moving along |
This allows users to set a common color for all items in the list without changing each item manually
This prevents users from needing to set a custom size on each element
mejiaj
suggested changes
Mar 3, 2021
Contributor
mejiaj
left a comment
There was a problem hiding this comment.
Overall very nice. Just have some questions about some possible fine-tuning.
| } | ||
| @if $prefix { | ||
| $this-type-scale: font-size($theme-icon-list-font-family, $token); | ||
| @include at-media($mq-key) { |
Contributor
There was a problem hiding this comment.
Could we be setting this @include at-media($mq-key) higher up to prevent duplication in output?
Example output
@media all and (min-width: 30em){
.mobile-lg\:usa-icon-list--size-xs .usa-icon-list__icon .usa-icon{
height:1.5rem;
margin-top:-1.5%;
width:1.5rem;
}
.mobile-lg\:usa-icon-list--size-xs .usa-icon-list__content{
font-size:1rem;
padding-left:0.4rem;
}
.mobile-lg\:usa-icon-list--size-xs .usa-icon-list__content > p,
.mobile-lg\:usa-icon-list--size-xs .usa-icon-list__content > h2,
.mobile-lg\:usa-icon-list--size-xs .usa-icon-list__content > h3,
.mobile-lg\:usa-icon-list--size-xs .usa-icon-list__content > h4,
.mobile-lg\:usa-icon-list--size-xs .usa-icon-list__content > h5,
.mobile-lg\:usa-icon-list--size-xs .usa-icon-list__content > h6,
.mobile-lg\:usa-icon-list--size-xs .usa-icon-list__content > ul,
.mobile-lg\:usa-icon-list--size-xs .usa-icon-list__content > ol,
.mobile-lg\:usa-icon-list--size-xs .usa-icon-list__content > div{
font-size:initial;
}
.mobile-lg\:usa-icon-list--size-xs .usa-icon-list__content .usa-icon-list__title{
font-size:1rem;
}
}
@media all and (min-width: 30em){
.mobile-lg\:usa-icon-list--size-sm .usa-icon-list__icon .usa-icon{
height:1.59rem;
margin-top:-1.5%;
width:1.59rem;
}
.mobile-lg\:usa-icon-list--size-sm .usa-icon-list__content{
font-size:1.06rem;
padding-left:0.424rem;
}
.mobile-lg\:usa-icon-list--size-sm .usa-icon-list__content > p,
.mobile-lg\:usa-icon-list--size-sm .usa-icon-list__content > h2,
.mobile-lg\:usa-icon-list--size-sm .usa-icon-list__content > h3,
.mobile-lg\:usa-icon-list--size-sm .usa-icon-list__content > h4,
.mobile-lg\:usa-icon-list--size-sm .usa-icon-list__content > h5,
.mobile-lg\:usa-icon-list--size-sm .usa-icon-list__content > h6,
.mobile-lg\:usa-icon-list--size-sm .usa-icon-list__content > ul,
.mobile-lg\:usa-icon-list--size-sm .usa-icon-list__content > ol,
.mobile-lg\:usa-icon-list--size-sm .usa-icon-list__content > div{
font-size:initial;
}
.mobile-lg\:usa-icon-list--size-sm .usa-icon-list__content .usa-icon-list__title{
font-size:1.06rem;
}
}
@media all and (min-width: 30em){
.mobile-lg\:usa-icon-list--size-md .usa-icon-list__icon .usa-icon{
height:1.695rem;
margin-top:-1.5%;
width:1.695rem;
}
.mobile-lg\:usa-icon-list--size-md .usa-icon-list__content{
font-size:1.13rem;
padding-left:0.452rem;
}
.mobile-lg\:usa-icon-list--size-md .usa-icon-list__content > p,
.mobile-lg\:usa-icon-list--size-md .usa-icon-list__content > h2,
.mobile-lg\:usa-icon-list--size-md .usa-icon-list__content > h3,
.mobile-lg\:usa-icon-list--size-md .usa-icon-list__content > h4,
.mobile-lg\:usa-icon-list--size-md .usa-icon-list__content > h5,
.mobile-lg\:usa-icon-list--size-md .usa-icon-list__content > h6,
.mobile-lg\:usa-icon-list--size-md .usa-icon-list__content > ul,
.mobile-lg\:usa-icon-list--size-md .usa-icon-list__content > ol,
.mobile-lg\:usa-icon-list--size-md .usa-icon-list__content > div{
font-size:initial;
}
.mobile-lg\:usa-icon-list--size-md .usa-icon-list__content .usa-icon-list__title{
font-size:1.13rem;
}
}
Contributor
There was a problem hiding this comment.
OK, I think i've got this one done
Contributor
There was a problem hiding this comment.
I still see duplicate media queries for different icon list sizes.
Something like this might fix it:
@each $mq-key, $mq-value in $icon-list-breakpoints {
$prefix: false;
@if $mq-key == "none" {
$prefix: "";
}
// Or the standard prefix if the breakpoint is output
@else if map-get($theme-utility-breakpoints, $mq-key) {
$prefix: "#{$mq-key}\\:";
}
@include at-media($mq-key) {
@each $token, $val in $theme-body-font-sizes {
@if $prefix {
$this-type-scale: font-size($theme-icon-list-font-family, $token);
.#{$prefix}usa-icon-list--size-#{$token} {
.usa-icon-list__icon {
.usa-icon {
// Set the height and width of the icon based on the size variant and factor
height: $this-type-scale * $icon-list-icon-size-factor;
width: $this-type-scale * $icon-list-icon-size-factor;
}
}
.usa-icon-list__content {
@include u-measure(5);
// Resize simple (un-marked up) content
font-size: size($theme-icon-list-font-family, $token);
// Calculate the space between the icon and content based on the size variant and factor
padding-left: $this-type-scale * $icon-list-icon-padding-left-factor;
.usa-icon-list__title {
@include u-font($theme-icon-list-title-font-family, $token);
}
}
}
}
}
}
}First I loop through the media queries and then I loop through the different sizes. Results in it being contained within a single media query.
Sample output
@media all and (min-width: 30em){
.mobile-lg\:usa-icon-list--size-xs .usa-icon-list__icon .usa-icon{
height:1.5rem;
width:1.5rem;
}
.mobile-lg\:usa-icon-list--size-xs .usa-icon-list__content{
max-width:72ex;
font-size:1rem;
padding-left:0.4rem;
}
.mobile-lg\:usa-icon-list--size-xs .usa-icon-list__content .usa-icon-list__title{
font-family:Merriweather Web, Georgia, Cambria, Times New Roman, Times, serif;
font-size:0.91rem;
}
.mobile-lg\:usa-icon-list--size-sm .usa-icon-list__icon .usa-icon{
height:1.59rem;
width:1.59rem;
}
.mobile-lg\:usa-icon-list--size-sm .usa-icon-list__content{
max-width:72ex;
font-size:1.06rem;
padding-left:0.424rem;
}
.mobile-lg\:usa-icon-list--size-sm .usa-icon-list__content .usa-icon-list__title{
font-family:Merriweather Web, Georgia, Cambria, Times New Roman, Times, serif;
font-size:0.98rem;
}
.mobile-lg\:usa-icon-list--size-md .usa-icon-list__icon .usa-icon{
height:1.695rem;
width:1.695rem;
}
.mobile-lg\:usa-icon-list--size-md .usa-icon-list__content{
max-width:72ex;
font-size:1.13rem;
padding-left:0.452rem;
}
.mobile-lg\:usa-icon-list--size-md .usa-icon-list__content .usa-icon-list__title{
font-family:Merriweather Web, Georgia, Cambria, Times New Roman, Times, serif;
font-size:1.04rem;
}
.mobile-lg\:usa-icon-list--size-lg .usa-icon-list__icon .usa-icon{
height:2.19rem;
width:2.19rem;
}
.mobile-lg\:usa-icon-list--size-lg .usa-icon-list__content{
max-width:72ex;
font-size:1.46rem;
padding-left:0.584rem;
}
.mobile-lg\:usa-icon-list--size-lg .usa-icon-list__content .usa-icon-list__title{
font-family:Merriweather Web, Georgia, Cambria, Times New Roman, Times, serif;
font-size:1.34rem;
}
.mobile-lg\:usa-icon-list--size-xl .usa-icon-list__icon .usa-icon{
height:3.195rem;
width:3.195rem;
}
.mobile-lg\:usa-icon-list--size-xl .usa-icon-list__content{
max-width:72ex;
font-size:2.13rem;
padding-left:0.852rem;
}
.mobile-lg\:usa-icon-list--size-xl .usa-icon-list__content .usa-icon-list__title{
font-family:Merriweather Web, Georgia, Cambria, Times New Roman, Times, serif;
font-size:1.95rem;
}
.mobile-lg\:usa-icon-list--size-2xl .usa-icon-list__icon .usa-icon{
height:3.99rem;
width:3.99rem;
}
.mobile-lg\:usa-icon-list--size-2xl .usa-icon-list__content{
max-width:72ex;
font-size:2.66rem;
padding-left:1.064rem;
}
.mobile-lg\:usa-icon-list--size-2xl .usa-icon-list__content .usa-icon-list__title{
font-family:Merriweather Web, Georgia, Cambria, Times New Roman, Times, serif;
font-size:2.44rem;
}
.mobile-lg\:usa-icon-list--size-3xl .usa-icon-list__icon .usa-icon{
height:4.785rem;
width:4.785rem;
}
.mobile-lg\:usa-icon-list--size-3xl .usa-icon-list__content{
max-width:72ex;
font-size:3.19rem;
padding-left:1.276rem;
}
.mobile-lg\:usa-icon-list--size-3xl .usa-icon-list__content .usa-icon-list__title{
font-family:Merriweather Web, Georgia, Cambria, Times New Roman, Times, serif;
font-size:2.93rem;
}
}
mejiaj
reviewed
Mar 3, 2021
…/uswds into accelerator/3772-icon-list
- Use `default` as the default for $theme-icon-list-font-size. This means that icon list will automatically take the size of body text ($theme-body-font-size) unless overridden - Add a $theme-icon-list-title-font-family to set the family of the item titles
- Use body default as standard font size, use variants only for changing size (not settings) - Simplify and improve icon alignment
Contributor
|
@mejiaj Can you look at this again? I think I've resolved everything you mentioned. |
mejiaj
reviewed
Mar 12, 2021
mejiaj
approved these changes
Mar 12, 2021
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
5 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Description
This is the initial commit of Icon list. It includes the method of including icons as developed by @jaredcunha inside the HTML as opposed to integrating icon bullets from within the .scss files (i.e. using the ::before pseudo element)
https://federalist-3b6ba08e-0df4-44c9-ac73-6fc193b0e19c.app.cloud.gov/preview/uswds/uswds/accelerator/3772-icon-list/components/detail/icon-list.html
Additional information
Before you hit Submit, make sure you’ve done whichever of these applies to you:
npm testand make sure the tests for the files you have changed have passed.