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

Round of grid cleanup #45

Merged
merged 10 commits into from
Aug 5, 2019
Merged

Round of grid cleanup #45

merged 10 commits into from
Aug 5, 2019

Conversation

ashley-hebler
Copy link
Member

@ashley-hebler ashley-hebler commented Aug 2, 2019

What's this PR do?

A step toward grid cleanup

Classes added (if any)
  • has-gutter-padding
  • has-temp-gutter-padding
  • has-section-padding
Classes removed (if any)
  • grid_padded
  • grid_padded--temp
  • grid_padded--{size} (5)
  • hide_{from || until}--{size} (10)

Why are we doing this? How does it help us?

Reduction in CSS (a whole 1.73kb, but still ¯_(ツ)_/¯ ) :

  • Trims down logic for adding gutter padding at various breakpoints. We seldom use grid_padded--{size} and those have a superfluous media query.
  • Removes the dynamic hide helpers, which we've already replaced thanks to @AndrewGibson27.
  • Removes sidebar column. This has no replacement yet, but we'll tackle that when it comes up during homepage CSS refactor.

How should this be manually tested?

yarn dev

  1. See: has-padding
    Resize the browser to see the gutter padding growing/shrinking

  2. Bonus new and improved legacy docs/demos (for no good reason, other than comparison):

Does this introduce a breaking change where queso-ui is used in the wild? If so, is there a relevant branch/PR to accompany this release?

It does. We rely on the hide classes in our queso-only pages in the header/footer. We also rely on grid_padded. This PR will remedy that.

TODOs / next steps:

  • Remove pre-release label associated with this and add 2.0.0 to tt repo

@ashley-hebler ashley-hebler changed the title Replace grid padded Round of grid cleanup Aug 2, 2019
@ashley-hebler ashley-hebler marked this pull request as ready for review August 5, 2019 13:26
@emilyyount
Copy link

@ashley-hebler: One more class to take a look at is .section_padded. I used it for the subscribe landing, strategic plan and support landing in instances where I wanted to have spacing between sections and I couldn't rely on margin classes because of the background color changing. Not sure how Andrew is handling in the portal.

https://www.texastribune.org/about/subscribe/
https://www.texastribune.org/about/texas-tribune-strategic-plan/
https://support.texastribune.org/donate

// grid_container => [**l-container**](#container-l-container) <br>
// grid_padded => [**has-gutter-padding**](/pages/utilities/index.html#padding-has-padding) <br>
// grid_padded--temp => [**has-temp-gutter-padding**](/pages/utilities/index.html#padding-has-padding) <br>
// col_adunit300x250 => Removed <br>
Copy link
Contributor

Choose a reason for hiding this comment

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

Just to confirm, this won't break non-story pages that rely on this, right? There are several that do.

Copy link
Member Author

Choose a reason for hiding this comment

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

In general there are only a few pages that go all in on this trimmed grid. grid_padded and grid_padded--temp are used on those three pages. I have a secondary PR in /texastribune to fix that once we settle this one.

@@ -56,4 +58,20 @@

.has-reset-padding {
Copy link
Contributor

Choose a reason for hiding this comment

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

Is this for elements that normalize.css fails to remove default user-agent padding from? If not, could we just remove this class?

Copy link
Member Author

Choose a reason for hiding this comment

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

Way back when, I needed some padding-less buttons. I thought maybe it'd be helpful to have this for instances where you just want to kill some padding on a component, but maybe we just leave that scoped to the component. I can create a padding-less button variation in that case.

@@ -56,4 +58,20 @@

.has-reset-padding {
padding: 0;
}

.has-gutter-padding {
Copy link
Contributor

Choose a reason for hiding this comment

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

Just a personal opinion, but when I think "gutter," I think about space between columns. This seems like it's more for adding space around an entire element as it relates to the edge of the window, not other elements. But feel free to override me. Regardless, this class will be very useful!

}
}

.has-temp-gutter-padding {
Copy link
Contributor

Choose a reason for hiding this comment

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

I never understood what "temp" meant. My guess is Becca intended eventually to remove it. Do you know? If it's a mystery, we might consider renaming.

@AndrewGibson27
Copy link
Contributor

Loving this; see a few notes above. Exciting to see the old grid world start to wither away!

@ashley-hebler
Copy link
Member Author

ashley-hebler commented Aug 5, 2019

@AndrewGibson27
That's fair on those names. Any thoughts on replacements for either? I had to add descriptions to the old to help me even understand where that padding kicks in. Essentially this is what they do:
Screen Shot 2019-08-05 at 2 18 14 PM

has-resp-padding | has-xl-resp-padding? Or has-x-padding-until-xxl has-x-padding-only-xl? They're weird because it's side padding, but only sometimes. Not sure what short word would indicate that.

@ashley-hebler
Copy link
Member Author

@emilyyount, good call on section_padded. Added that to our padding helpers too. @AndrewGibson27, does has-section-padding sit right with you?

@ashley-hebler
Copy link
Member Author

Chatted with @AndrewGibson27 a bit on slack and went with has-page-padding and has-page-padding-at-bp-l for those classes in question. Merging and bumping us up to 2.0.0. Thanks for the help!
cc @emilyyount

@ashley-hebler ashley-hebler merged commit d107c45 into master Aug 5, 2019
@ashley-hebler ashley-hebler deleted the replace-grid-padded branch August 6, 2019 18:35
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.

3 participants