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

Updates to marketing utilities for positioning elements #619

Merged
merged 18 commits into from
Jan 23, 2019

Conversation

gladwearefriends
Copy link
Contributor

@gladwearefriends gladwearefriends commented Dec 1, 2018

This updates primer-marketing-utilities with new positioning utilities based on our new marketing site design styles.

We often need to absolutely position svgs and images in our new design, some examples...
image

image

To do this we have been using .position-absolute/.position-relative along with new utility classes as left-6 or top-lg-12.

This PR

  • adds those utilities to allow positioning from all 4 edges of the element (top, right, bottom, left), and supports all spacing values (spacing-1 to spacing-12) through all responsive sizes (sm to xl)
  • adds new variable $allSpacers to primer-marketing-support as well
  • updates stories.js and the docs

/cc @primer/ds-core @trosage @skullface

Copy link
Contributor

@shawnbot shawnbot left a comment

Choose a reason for hiding this comment

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

Hi @gladwearefriends! There's a new way to do responsive utilities that allows you to write the rules once rather than one time outside of the @for and another inside it. Check out #545 for the 411, and let me know if you run into any problems or have questions!

@gladwearefriends
Copy link
Contributor Author

awesome! ive updated it to use the new breakpoints mixin! thank you @shawnbot

@mdo
Copy link
Contributor

mdo commented Jan 4, 2019

One thing I've run into while working on pricing changes and I was curious how much CSS was being generated (largely because it keeps timing out locally). Turns out the positioning utilities generate a ton of CSS, so much so that we have over 400 unused selectors that are whitelisted in the compiled CSS for our selectors_match_markup test.

Can we evaluate our needs a bit here to keep the CSS down?

@shawnbot
Copy link
Contributor

shawnbot commented Jan 4, 2019

Oof, sorry @mdo. Yes, y'all should definitely evaluate what you need. Let us know what we can do to support modifying the scope of your utilities on a site-by-site basis (if that seems like something that would help).

@gladwearefriends
Copy link
Contributor Author

gladwearefriends commented Jan 10, 2019

Thank you for bringing up the css bloat issue @mdo! Ive gone through and updated thisremoving unnecessary util classes, classes we rarely or will probably never need. cc/ @primer/github-site-design

What i did:

  • remove all negative 0 utils, i accidentally included these in the sass map 😅 no such thing as negative 0 (e.g. bottom-n0) - this removes 20 utils
  • keep all breakpoint-less utils (e.g. bottom-5 or bottom-n5): allows us to edit positioning for all breakpoints, and starting at the smallest viewport width. these are the most popular of the position utils we use.
  • remove all -sm breakpoint utils: we have never used a single -sm util yet which makes sense. probably rare we will need to adjust positioning at small breakpoint - this removes 104 utils
  • keep all -md and -lg utils: we currently only use a quarter of these utils but we do use them to adjust positioning at these important layout changing breakpoints
  • remove all -xl breakpoint utils: we currently only use 2 of these utils and by removing these we remove another 104 utils. makes sense that adjusting positioning from lg to xl breakpoint is rare and we should just aim to get the ideal positioning at the lg breakpoint

To do this I created a new $responsive-variants-positionUtils variable that omits the sm and xl breakpoints @shawnbot. Is this a good way to accomplish this?

@mdo
Copy link
Contributor

mdo commented Jan 10, 2019

Yes! Thank you so much for digging into this and reporting back your findings and changes. <3

@shawnbot
Copy link
Contributor

@gladwearefriends yes, that's totally fine! I can fix the merge conflict in package-lock.json if you'd like, then we can roll this into a release soon. ❤️

@primer primer deleted a comment Jan 15, 2019
@shawnbot shawnbot changed the base branch from master to release-11.0.0 January 23, 2019 19:37
@shawnbot shawnbot dismissed their stale review January 23, 2019 22:57

no longer relevant

@shawnbot

This comment has been minimized.

@shawnbot
Copy link
Contributor

@gladwearefriends okay, now I've got negative values in there too and here is the updated list. It's... um, extensive at 288 new selectors. Does this look right to you? 😬

1a2,73
> .bottom-1
> .bottom-10
> .bottom-11
> .bottom-12
> .bottom-2
> .bottom-3
> .bottom-4
> .bottom-5
> .bottom-6
> .bottom-7
> .bottom-8
> .bottom-9
> .bottom-lg-1
> .bottom-lg-10
> .bottom-lg-11
> .bottom-lg-12
> .bottom-lg-2
> .bottom-lg-3
> .bottom-lg-4
> .bottom-lg-5
> .bottom-lg-6
> .bottom-lg-7
> .bottom-lg-8
> .bottom-lg-9
> .bottom-lg-n1
> .bottom-lg-n10
> .bottom-lg-n11
> .bottom-lg-n12
> .bottom-lg-n2
> .bottom-lg-n3
> .bottom-lg-n4
> .bottom-lg-n5
> .bottom-lg-n6
> .bottom-lg-n7
> .bottom-lg-n8
> .bottom-lg-n9
> .bottom-md-1
> .bottom-md-10
> .bottom-md-11
> .bottom-md-12
> .bottom-md-2
> .bottom-md-3
> .bottom-md-4
> .bottom-md-5
> .bottom-md-6
> .bottom-md-7
> .bottom-md-8
> .bottom-md-9
> .bottom-md-n1
> .bottom-md-n10
> .bottom-md-n11
> .bottom-md-n12
> .bottom-md-n2
> .bottom-md-n3
> .bottom-md-n4
> .bottom-md-n5
> .bottom-md-n6
> .bottom-md-n7
> .bottom-md-n8
> .bottom-md-n9
> .bottom-n1
> .bottom-n10
> .bottom-n11
> .bottom-n12
> .bottom-n2
> .bottom-n3
> .bottom-n4
> .bottom-n5
> .bottom-n6
> .bottom-n7
> .bottom-n8
> .bottom-n9
2a75,146
> .left-1
> .left-10
> .left-11
> .left-12
> .left-2
> .left-3
> .left-4
> .left-5
> .left-6
> .left-7
> .left-8
> .left-9
> .left-lg-1
> .left-lg-10
> .left-lg-11
> .left-lg-12
> .left-lg-2
> .left-lg-3
> .left-lg-4
> .left-lg-5
> .left-lg-6
> .left-lg-7
> .left-lg-8
> .left-lg-9
> .left-lg-n1
> .left-lg-n10
> .left-lg-n11
> .left-lg-n12
> .left-lg-n2
> .left-lg-n3
> .left-lg-n4
> .left-lg-n5
> .left-lg-n6
> .left-lg-n7
> .left-lg-n8
> .left-lg-n9
> .left-md-1
> .left-md-10
> .left-md-11
> .left-md-12
> .left-md-2
> .left-md-3
> .left-md-4
> .left-md-5
> .left-md-6
> .left-md-7
> .left-md-8
> .left-md-9
> .left-md-n1
> .left-md-n10
> .left-md-n11
> .left-md-n12
> .left-md-n2
> .left-md-n3
> .left-md-n4
> .left-md-n5
> .left-md-n6
> .left-md-n7
> .left-md-n8
> .left-md-n9
> .left-n1
> .left-n10
> .left-n11
> .left-n12
> .left-n2
> .left-n3
> .left-n4
> .left-n5
> .left-n6
> .left-n7
> .left-n8
> .left-n9
182a327,470
> .right-1
> .right-10
> .right-11
> .right-12
> .right-2
> .right-3
> .right-4
> .right-5
> .right-6
> .right-7
> .right-8
> .right-9
> .right-lg-1
> .right-lg-10
> .right-lg-11
> .right-lg-12
> .right-lg-2
> .right-lg-3
> .right-lg-4
> .right-lg-5
> .right-lg-6
> .right-lg-7
> .right-lg-8
> .right-lg-9
> .right-lg-n1
> .right-lg-n10
> .right-lg-n11
> .right-lg-n12
> .right-lg-n2
> .right-lg-n3
> .right-lg-n4
> .right-lg-n5
> .right-lg-n6
> .right-lg-n7
> .right-lg-n8
> .right-lg-n9
> .right-md-1
> .right-md-10
> .right-md-11
> .right-md-12
> .right-md-2
> .right-md-3
> .right-md-4
> .right-md-5
> .right-md-6
> .right-md-7
> .right-md-8
> .right-md-9
> .right-md-n1
> .right-md-n10
> .right-md-n11
> .right-md-n12
> .right-md-n2
> .right-md-n3
> .right-md-n4
> .right-md-n5
> .right-md-n6
> .right-md-n7
> .right-md-n8
> .right-md-n9
> .right-n1
> .right-n10
> .right-n11
> .right-n12
> .right-n2
> .right-n3
> .right-n4
> .right-n5
> .right-n6
> .right-n7
> .right-n8
> .right-n9
> .top-1
> .top-10
> .top-11
> .top-12
> .top-2
> .top-3
> .top-4
> .top-5
> .top-6
> .top-7
> .top-8
> .top-9
> .top-lg-1
> .top-lg-10
> .top-lg-11
> .top-lg-12
> .top-lg-2
> .top-lg-3
> .top-lg-4
> .top-lg-5
> .top-lg-6
> .top-lg-7
> .top-lg-8
> .top-lg-9
> .top-lg-n1
> .top-lg-n10
> .top-lg-n11
> .top-lg-n12
> .top-lg-n2
> .top-lg-n3
> .top-lg-n4
> .top-lg-n5
> .top-lg-n6
> .top-lg-n7
> .top-lg-n8
> .top-lg-n9
> .top-md-1
> .top-md-10
> .top-md-11
> .top-md-12
> .top-md-2
> .top-md-3
> .top-md-4
> .top-md-5
> .top-md-6
> .top-md-7
> .top-md-8
> .top-md-9
> .top-md-n1
> .top-md-n10
> .top-md-n11
> .top-md-n12
> .top-md-n2
> .top-md-n3
> .top-md-n4
> .top-md-n5
> .top-md-n6
> .top-md-n7
> .top-md-n8
> .top-md-n9
> .top-n1
> .top-n10
> .top-n11
> .top-n12
> .top-n2
> .top-n3
> .top-n4
> .top-n5
> .top-n6
> .top-n7
> .top-n8
> .top-n9

@shawnbot shawnbot merged commit c9e70d2 into release-11.0.0 Jan 23, 2019
@shawnbot shawnbot deleted the marketing-position-utils-2018 branch January 23, 2019 23:24
@jonrohan jonrohan mentioned this pull request Jan 23, 2019
22 tasks
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.

None yet

3 participants