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

Cut down on media queries generated by @modular-spacing #343

Merged
merged 3 commits into from
Mar 19, 2019

Conversation

yvonnetangsu
Copy link
Member

@yvonnetangsu yvonnetangsu commented Mar 14, 2019

READY FOR REVIEW

Summary

  • @modular-spacing use a for loop to generate media queries for every breakpoint so total of 6 queries per element we use the mixin on.
  • xs and sm shared the same base value, md, lg, xl shared another one, so we really only need 3 media queries for each element that uses @modular-spacing
  • Propose one more variable map for media queries for use with modular-spacing so we can cut the number of queries from 6 to 3 for every use.

Needed By (Date)

  • Whenever

Urgency

  • Not Urgent

Steps to Test

Look at the "Files changed" tabs here on this GitHub page. You should be able to see in decanter.css the deleted media queries that are not really needed.

OR

  1. Pull this branch and run grunt styleguide
  2. Look at .su-global-footer in decanter.css - it should now only have 3 media queries for the padding-top and padding-bottom instead of 6.

Affected Projects or Products

  • Decanter

@yvonnetangsu
Copy link
Member Author

@sherakama @josephgknox Yay or nay? Not a big deal either way, but as we use @modular-spacing more and more, this could cut down on some redundancy.

Copy link
Member

@sherakama sherakama left a comment

Choose a reason for hiding this comment

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

Unfortunately, as much as I like this optimization it breaks the ability for one to modify the grid-system easily. This is creating a new grid system with only 3 values where if I had made changes to the $grid-media global I would have expected it to have cascaded everywhere. I don't think we can make this assumption.

@yvonnetangsu
Copy link
Member Author

Unfortunately, as much as I like this optimization it breaks the ability for one to modify the grid-system easily. This is creating a new grid system with only 3 values where if I had made changes to the $grid-media global I would have expected it to have cascaded everywhere. I don't think we can make this assumption.

Aww...that's ok. Worth a try 😃 . Do you think there's any other way we can achieve the same results? Manually writing out the 3 queries in the @modular-spacing mixin maybe or too high maintenance?

@sherakama
Copy link
Member

Perhaps you could calculate the result value before outputting the media query and compare it to the previous value?

…write media query if modular base is not the same as previous breakpoint
@yvonnetangsu yvonnetangsu requested a review from sherakama March 14, 2019 22:42
@yvonnetangsu
Copy link
Member Author

Perhaps you could calculate the result value before outputting the media query and compare it to the previous value?

What do you think, @sherakama ? I have it retrieve current breakpoint's modular-spacing base and compare to the one of the previous breakpoint, and write output media query only if they're not the same (and for the first breakpoint also). It generates the same smaller CSS output with the 3 media query now.

Copy link
Member

@sherakama sherakama left a comment

Choose a reason for hiding this comment

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

Thanks for the revision. This looks good to me.

During my testing I modified the grid-media array and broke the sass compiler somewhere else. So we have a bug in our code that doesn't allow for changing the grid system.

Let me create a ticket for that debug as this is GTG.

@yvonnetangsu
Copy link
Member Author

Thanks for the review and the tweaks, @sherakama ! Feel free to merge if you think it's ready - depending on if you think @josephgknox needs to take a second look as well. I know reviewing takes time so don't want to request additional reviews if not necessary 😺

I'll make changes to the other branches as you suggested on Monday and thanks for sending that Zoom invite.

@yvonnetangsu yvonnetangsu changed the title Add grid-media-modular to cut down on media queries generated by @modular-spacing Cut down on media queries generated by @modular-spacing Mar 18, 2019
@josephgknox
Copy link

Thank you two (again) for the hard work on this one!

Copy link

@josephgknox josephgknox left a comment

Choose a reason for hiding this comment

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

G2G!

@josephgknox josephgknox merged commit 172b197 into master Mar 19, 2019
@josephgknox josephgknox deleted the media-query-modular branch March 19, 2019 18:34
yvonnetangsu added a commit that referenced this pull request Mar 20, 2019
* master:
  Cut down on media queries generated by @modular-spacing (#343)
JBCSU added a commit that referenced this pull request Mar 20, 2019
* master:
  Cut down on media queries generated by @modular-spacing (#343)
  Main Nav Light on Dark Background Color Variant (#344)

# Conflicts:
#	core/dist/css/decanter.css - resolved using master's, then rebuilt with webpack
yvonnetangsu added a commit that referenced this pull request Mar 20, 2019
* master:
  Use %placeholder and @extend on button mixins to see if it makes code less repetitive (#347)
  Cut down on media queries generated by @modular-spacing (#343)
  Main Nav Light on Dark Background Color Variant (#344)
  Reorganize files for CTA so it's consistent with other components (#345)
  Update main-nav.twig (#331)
  get rid of ID (#342)
  5 Lines Lockup + Variants (#289)
  Update _alert--info.scss (#330)

# Conflicts:
#	core/css/decanter.css
#	core/scss/utilities/mixins/button/_button-big.scss
#	core/scss/utilities/mixins/button/_button-primary.scss
#	core/scss/utilities/mixins/button/_button-secondary.scss
#	core/scss/utilities/mixins/index.scss
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants