Skip to content

Conversation

@mitchmoccia
Copy link
Contributor

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

flexion-icon-list-sample1

Before you hit Submit, make sure you’ve done whichever of these applies to you:

  • Follow the 18F Front End Coding Style Guide and Accessibility Guide.
  • Run npm test and make sure the tests for the files you have changed have passed.
  • Run your code through HTML_CodeSniffer and make sure it’s error free.
  • Title your pull request using this format: [Website] - [UI component]: Brief statement describing what this pull request solves.

@thisisdano
Copy link
Contributor

woohoo — good to see this moving along

@thisisdano thisisdano changed the title Accelerator/3772 icon list - initial commit Icon list component Dec 9, 2020
@thisisdano thisisdano changed the title Icon list component Add icon list component Mar 1, 2021
@thisisdano thisisdano changed the base branch from develop to uswds-2.11.0 March 1, 2021 20:55
@thisisdano thisisdano requested review from mejiaj and thisisdano March 1, 2021 20:55
@thisisdano thisisdano mentioned this pull request Mar 1, 2021
34 tasks
@mejiaj mejiaj requested a review from scottqueen-bixal March 2, 2021 18:57
Copy link
Contributor

@mejiaj mejiaj left a comment

Choose a reason for hiding this comment

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

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) {
Copy link
Contributor

Choose a reason for hiding this comment

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

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;
}
}

Copy link
Contributor

Choose a reason for hiding this comment

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

OK, I think i've got this one done

Copy link
Contributor

@mejiaj mejiaj Mar 12, 2021

Choose a reason for hiding this comment

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

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;
  }
}

Copy link
Contributor

Choose a reason for hiding this comment

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

ahh, I get you now! 🤦

- 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
@thisisdano thisisdano requested a review from mejiaj March 11, 2021 21:44
- Use body default as standard font size, use variants only for changing size (not settings)
- Simplify and improve icon alignment
@thisisdano
Copy link
Contributor

@mejiaj Can you look at this again? I think I've resolved everything you mentioned.

Copy link
Contributor

@mejiaj mejiaj left a comment

Choose a reason for hiding this comment

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

Just added a note about the media query duplication and icon color setting in YML. Otherwise LGTM

@mejiaj mejiaj merged commit c1c0247 into uswds-2.11.0 Mar 12, 2021
@mejiaj mejiaj deleted the accelerator/3772-icon-list branch March 12, 2021 19:16
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Add icon list component

6 participants