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

Refactored container for simplicity #586

Merged
merged 6 commits into from
Dec 31, 2021
Merged

Refactored container for simplicity #586

merged 6 commits into from
Dec 31, 2021

Conversation

palewire
Copy link
Collaborator

@palewire palewire commented Dec 30, 2021

This pull request replaces the container logic of bootstrap, and our numerous hacks on top of it, with a less complex system. Pages should look virtually the same afterward.

Here's what it does:

  • Creates a new _containers component
  • Adds a new set of content classes that handle max-width separately from the container
  • Reduces the max-width for text blocks to a narrower, more readable extent in line with mainstream publishers
  • Removes numerous negative margin and padding hacks
  • Removes unused page_content type classes
  • Switched the About page to an HTML template to make it work and to tee up future refactoring

@palewire palewire added this to the Streamline the site milestone Dec 30, 2021
@choldgraf
Copy link
Collaborator

This sounds great to ke - my main question is what logic we are losing from the bootstrap helper class. I guess there was just a bunch of stuff we weren't using?

Also we should totally do something like this for the jupyter book and pydata sphinx themes...

@palewire
Copy link
Collaborator Author

palewire commented Dec 30, 2021

The bootstrap container used a media query to set different max widths for all content at different viewports. Instead, I've defined two fixed max widths, a narrower one for copy and a wider one for art. Both will fill 100% at mobile devices. Pretty subtle, but helps make the code more declarative and consistent.

Bootstrap also had some crazy negative margin stuff I cut.

@palewire palewire linked an issue Dec 31, 2021 that may be closed by this pull request
@palewire
Copy link
Collaborator Author

This will fix #541

Copy link
Collaborator

@choldgraf choldgraf left a comment

Choose a reason for hiding this comment

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

sorry for the slow turnaround, was traveling back to California

This looks good to me - I like that we are simplifying our CSS and HTML a bit, removing unnecessary rules and also removing dependencies on Bootstrap. The new width for copy sounds reasonable to me, I think it's a nice improvement.

@palewire I see you've been pushing more updates to this PR, is there something else you want to finish up or is this good to go?

@palewire
Copy link
Collaborator Author

It's good to go. I just killed some cruft.

@palewire palewire merged commit 9956b8c into master Dec 31, 2021
@krassowski krassowski deleted the tidy-container branch December 31, 2021 23:37
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.

Container padding on mobile is wider than it needs to be
2 participants