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.

Sign up for GitHub

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

Additional padding and margin settings #1110

Merged
merged 15 commits into from
Jan 12, 2022
Merged

Additional padding and margin settings #1110

Changes from 1 commit
Commits
Show all changes
15 commits
Select commit Hold shift + click to select a range
3884d11
Additional padding and margin settings
tyleralsbury Jan 6, 2022
162dd27
Fixed vertical spacing for collapisble and slideshow
tyleralsbury Jan 7, 2022
0e33490
Adjustments
tyleralsbury Jan 7, 2022
0dc2b82
Removed bottom margin on grid when it is the last element
tyleralsbury Jan 7, 2022
bffe563
Adjusted class for collapsible content
tyleralsbury Jan 7, 2022
40d7ddd
Update 15 translation files
translation-platform[bot] Jan 10, 2022
fb95aa0
Refactor padding calc
tyleralsbury Jan 10, 2022
59d64c9
Merge branch 'padding-follow-up' of https://github.com/Shopify/dawn i…
tyleralsbury Jan 10, 2022
3789d7e
Fixed up a few paddings
tyleralsbury Jan 10, 2022
dcbab5b
CSS class naming
tyleralsbury Jan 10, 2022
21ce860
Update 5 translation files
translation-platform[bot] Jan 11, 2022
6ee4afa
Removed unneeded styles
tyleralsbury Jan 11, 2022
59ed2cb
Collapsible content colour order
tyleralsbury Jan 11, 2022
960d11f
Fixed margins on headings
tyleralsbury Jan 11, 2022
d33b7a3
Nitpicks
tyleralsbury Jan 12, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 4 additions & 0 deletions assets/base.css
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -982,6 +982,10 @@ summary::-webkit-details-marker {
}
}

.grid:last-child {
margin-bottom: 0;
}
Copy link
Contributor

Choose a reason for hiding this comment

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

Curious to know which ones we had as an offender here ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This accounts for any case where .grid is the last element - looking through the code it seems like this is often the case. The only thing that it does cause, which might be an issue, is that if it's the last child of some parent element that has something else after it it might not be great.

I'm going to go through and see if that's the case anywhere. I notice that it does squish the margin between the cards in Featured Collection and the View all button.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Turns out there's a lot of inconsistencies with those types of buttons...

I've added some spacing for the one for Featured Collection cause it was wonky looking, but just looking at the remaining ones:

// Blog view all button

.blog__button {
  margin-top: 3rem;
}

@media screen and (min-width: 750px) {
  .blog__button {
    margin-top: 5rem;
  }
}

// Multicolumn

.multicolumn .button {
  margin-top: 1.5rem;
}

@media screen and (min-width: 750px) {
  .multicolumn .button {
    margin-top: 4rem;
  }
}

And both collection list and featured collection had just 2rem. I think changing them all to 2rem all the time would be good for consistency, and would potentially reduce CSS if we considered them to be a "button at the bottom of a section" component (naming is hard) and just used a class for all of them.

Copy link
Contributor

Choose a reason for hiding this comment

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

We could look into why the grid component (which is used in a variety of scenarios) imposes a 2rem bottom margin in the first place. I'd be in favour of setting .grid to a default zero top and bottom margin and only add margin in cases where it's really needed. That would require testing each scenario, so probably out of scope for this PR.


.grid__item {
padding-left: var(--grid-mobile-horizontal-spacing);
padding-bottom: var(--grid-mobile-vertical-spacing);
Expand Down