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’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add icon list component #3876

Merged
merged 93 commits into from
Mar 12, 2021
Merged

Add icon list component #3876

merged 93 commits into from
Mar 12, 2021

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
Member

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 requested a review from mejiaj March 1, 2021 20:55
@thisisdano thisisdano mentioned this pull request Mar 1, 2021
34 tasks
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.

src/stylesheets/settings/_settings-components.scss Outdated Show resolved Hide resolved
src/components/icon-list/icon-list.njk Outdated Show resolved Hide resolved
src/components/icon-list/icon-list.njk Outdated Show resolved Hide resolved
src/stylesheets/components/_icon-list.scss Show resolved Hide resolved
}
@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
Member

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
Member

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
Member

@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
5 participants