Skip to content
This repository has been archived by the owner on Nov 18, 2021. It is now read-only.

Replace mega hack with mini hack. #66

Merged
merged 6 commits into from Feb 1, 2019
Merged

Conversation

notlee
Copy link
Contributor

@notlee notlee commented Jan 25, 2019

Makes the aside-sidebar of the query layout optional.

I think this is better for users than the wrapper approach and the previous modifier approach because it requires no additional markup. And we get to remove the _getMainMaxWidth mega hack 馃帀

Unrelated this PR also corrects footer markup, and updates the migration guide accordingly.

Good

  • Simplifies the SCSS (removes _getMainMaxWidth super grossness).
  • Makes the css grid more flexible as the main content can be expressed in fractions rather than px.
  • To not use a sidebar on the query layout, just don't include the sidebar element.
  • Easy to document and understand.

Bad

  • The grid gutter for the sidebar takes up space even when the sidebar is empty.

screenshot 2019-01-25 at 17 02 21

@notlee notlee requested a review from a team as a code owner January 25, 2019 16:58
This was referenced Jan 25, 2019
@notlee
Copy link
Contributor Author

notlee commented Jan 25, 2019

There's no mega hack but there is a mini hack 馃槄 I'll add code comments on Monday
left: #{calc((100vw - 100%) * -0.5)};

@notlee
Copy link
Contributor Author

notlee commented Jan 28, 2019

Our modivation is to support registry-like layouts and layouts like that pioneered by bizops.

screenshot 2019-01-28 at 11 56 49
screenshot 2019-01-28 at 11 57 39

Copy link
Contributor

@gvonkoss gvonkoss left a comment

Choose a reason for hiding this comment

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

I may have missed something in one of the last PRs, but I have question 馃憞 and some smol comments 鉁岋笍

I like the non modifier non wrapper business though!

README.md Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
src/scss/_grid.scss Show resolved Hide resolved
// Relative to the center aligned grid.
position: relative;
width: 100vw;
left: #{calc((100vw - 100%) * -0.5)};
Copy link
Contributor

Choose a reason for hiding this comment

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

So this is happening because the grid doesn't span the whole page, right? Why did we remove the five columns again?

Copy link
Contributor Author

@notlee notlee Feb 1, 2019

Choose a reason for hiding this comment

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

Yeah, it's to remove the mega hack which calculated in SCSS a px value for the main content area. We either need a wrapper around the main content and sidebar, or we set the whole of .o-layout to the max width of the container and then make the header and footer "break out" of the container (which is what this is).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ah, and the mega hack is bad because if the main content area is set in px it can't expand when the sidebar is deleted.

gvonkoss and others added 2 commits February 1, 2019 12:52
Co-Authored-By: notlee <notlee@users.noreply.github.com>
@notlee notlee merged commit ddd11ef into v3.0.0 Feb 1, 2019
@notlee notlee deleted the query-sidebar-optional-3 branch February 1, 2019 13:09
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants