Fix the Page section's width. #2364
Conversation
There was a problem hiding this comment.
Thank you Eugene!
The width is good but now I am not sure why we are not able to inherit the padding setting anymore, when I add multiple page section, you can notice that the padding setting is not rendering. (Video reference)
assets/base.css
Outdated
| @@ -248,7 +248,7 @@ html.no-js .no-js-hidden { | |||
| padding: 0 1.5rem; | |||
| } | |||
|
|
|||
| .page-width:not(.drawer-menu) { | |||
| .page-width:where(:not(.drawer-menu)) { | |||
There was a problem hiding this comment.
I may be missing some context from the drawer menu PR, but I'm not sure if I understand the inclusion of :not(.drawer-menu) in this context. The page-width is a very generic class that is meant to be used across all elements on the page, not limited to the header, so increasing its specificity by adding a :not() to target a section-specific element sounds a bit off.
If our goal was to have drawer-menu take up to 100% of the max-width, can we include a class like this:
/* or maybe just .drawer-menu */
.page-width.drawer-menu {
max-width: 100%;
}instead, and keep .page-width as it was before? To me, that would make it a bit more clear as to why this element shouldn't have a max-width
As for the use of where(), this matches the example of it in the specs so I'm totally aligned with the solution you proposed :DD 🎉 I'm just not sure if its need could be avoided in the first place.
There was a problem hiding this comment.
cc @ludoboludo do you have any thoughts on that?
Eugene tried the solution I suggested (override on the specific drawer-menu, rather than rewriteing the selector for the generic page-width) and it seems to produce the same results.
I like both solutions - it's just that the one using where() feels like the problem could be avoided in the first place if we didn't rewrite the generic selector.
There was a problem hiding this comment.
I like your take on it 👍 Easier and simpler to read
eugenekasimov
dismissed stale reviews from ludoboludo and melissaperreault
via
175b27c
eugenekasimov
force-pushed
the
fix-page-section-width
branch
from
d357131
to
175b27c
Compare
eugenekasimov
requested review from
ludoboludo,
LucasLacerdaUX and
melissaperreault
* shopify/main: Improve image sizes in the multicolumn section (Shopify#2349) Fix the Page section's width. (Shopify#2364) Update 12 translation files (Shopify#2366) Removing "my" from cart popup notification (Shopify#2353) [Cart.js] Fix fetch url so it's not hard coded (Shopify#2357) (Shopify#2365) Update 1 translation file (Shopify#2352) Default Follow on Shop to on [Header] Add localization selectors (Shopify#2258) Remove async CSS pattern where it may introduce layout shifts (Shopify#2270) Change rich text section heading to be of type inline_richtext, also moved rte styling into base.css (Shopify#2326) Add drawer menu desktop (Shopify#2195) Make header image preload and proper width (Shopify#2307)
* Increase specificity for page-width--narrow class * Add :where to lower specificity. * Minor refactor * Minor refactor
PR Summary:
Fixes an issue when the page sections looses its width reference.
Why are these changes introduced?
Fixes #2359.
What approach did you take?
I lowered specificity for
.page-width:not(.drawer-menu)class to its original one (0-1-0) by changing it to.page-width:where(:not(.drawer-menu))Visual impact on existing themes
The page section should be looking narrow now as it used to be.
Testing steps/scenarios
Demo links
Checklist