-
Notifications
You must be signed in to change notification settings - Fork 3.4k
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
[Header] add setting to tweak top and bottom padding #1654
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -71,6 +71,11 @@ | |
</style> | ||
|
||
{%- style -%} | ||
.header { | ||
padding-top: {{ section.settings.padding_top | times: 0.5 | round: 0 }}px; | ||
padding-bottom: {{ section.settings.padding_bottom | times: 0.5 | round: 0 }}px; | ||
} | ||
|
||
.section-header { | ||
margin-bottom: {{ section.settings.margin_bottom | times: 0.75 | round: 0 }}px; | ||
} | ||
|
@@ -80,6 +85,13 @@ | |
margin-bottom: {{ section.settings.margin_bottom }}px; | ||
} | ||
} | ||
|
||
@media screen and (min-width: 990px) { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. And here we've used There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This should be 990px, it's where the change of layout from hamburger to inline happens :D There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I don't feel like there is a specific win from doing it on There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I'll unresolve the conversation so we can get more thoughts :D I prefer to keep mobile + tablet with consistent spacing as they look the same (hamburger), instead of aiming to be consistent with other sections that look and behave differently from the header. |
||
.header { | ||
padding-top: {{ section.settings.padding_top }}px; | ||
padding-bottom: {{ section.settings.padding_bottom }}px; | ||
} | ||
} | ||
{%- endstyle -%} | ||
|
||
<script src="{{ 'details-disclosure.js' | asset_url }}" defer="defer"></script> | ||
|
@@ -826,6 +838,30 @@ | |
"unit": "px", | ||
"label": "t:sections.header.settings.margin_bottom.label", | ||
"default": 0 | ||
}, | ||
{ | ||
"type": "header", | ||
"content": "t:sections.all.padding.section_padding_heading" | ||
}, | ||
{ | ||
"type": "range", | ||
"id": "padding_top", | ||
"min": 4, | ||
"max": 36, | ||
"step": 4, | ||
"unit": "px", | ||
"label": "t:sections.all.padding.padding_top", | ||
"default": 20 | ||
}, | ||
{ | ||
"type": "range", | ||
"id": "padding_bottom", | ||
"min": 4, | ||
"max": 36, | ||
"step": 4, | ||
"unit": "px", | ||
"label": "t:sections.all.padding.padding_bottom", | ||
"default": 20 | ||
} | ||
] | ||
} | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We're using
times: 0.75
in other sections it seems. I feel like despite the reason you mention it might be better to keep the same approach ? 🤔Unless we want to stick to those original values I guess. But it would just be off by a few pixels so I think it should be fine.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would personally keep it as
0.5
here. A sticky header that is taller has a bigger impact than a section with extra padding at the top, so I think it makes sense to go with a smaller value here. As long as we keep using the same multiples (4px
), I personally think it's better to have a header that is not too tall than be consistent.But I'll keep this conversation unresolved so UX can share their thoughts and make a decision.