-
Notifications
You must be signed in to change notification settings - Fork 3.6k
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
Louisa mobile drawer UI fix #2695
Conversation
Screenshot of everything lining up Video showing mobile drawer with mobile inspector |
Hi Louisa! I just recorded a piece of feedback in that video! |
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.
Looking good 👍
Left a few comments
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.
One minor tweak we could make (if it doesn't cause any trouble), but this otherwise looks approvable to me 👍
assets/component-menu-drawer.css
Outdated
padding: 1rem 0rem; | ||
font-size: 1.4rem; | ||
color: rgb(var(--color-foreground)); | ||
margin-bottom: 2rem; | ||
margin-bottom: 0rem; |
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.
Nitpicks padding: 1rem 0;
and margin-bottom: 0;
There's another 0 instance up higher in this file as well.
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.
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 just meant to remove the "rem" itself from the 0rem
values as its redundant and we typically just use 0
in these cases.
@@ -90,7 +90,7 @@ details[open].menu-opening > .menu-drawer__submenu { | |||
} | |||
|
|||
.menu-drawer__navigation { | |||
padding: 5.6rem 0; | |||
padding: 3rem 0; | |||
} | |||
|
|||
.menu-drawer__inner-submenu { |
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.
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.
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.
Oops, sorry just shared the same thing on my review just now. Basically here, it was the margin in your commit that was changed instead of the padding value :)
assets/component-menu-drawer.css
Outdated
@@ -202,7 +202,7 @@ details[open].menu-opening > .menu-drawer__submenu { | |||
|
|||
.menu-drawer__close-button .icon-arrow { | |||
transform: rotate(180deg); | |||
margin-right: 1rem; | |||
margin-right: 3rem; |
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 don't believe this should be changed 🤔 It seem to push away the word(s) next to it.
It's the .menu-drawer__close-button
that needs some tweaking. It currently have this padding value:
padding: 1.2rem 2.6rem;
which should be tweaked to
padding: 1.2rem 2.6rem 1.2rem 3rem;
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.
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.
Sorry! I misunderstood , I made the suggestions as suggested! Thank you both :)
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.
It's looking good to me 👍
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.
Perfect, thanks Louisa!
* added horizontal and vertical padding in Mobile drawer * vertical padding and horizontal padding work with large lists-need to fix padding w/out scrollbar * all icons, links are aligned with the close button and shop icon on mobile * horizontal and vertical paddings all equate to 3rem around Nav and utility links * added padding to close button on l3 links, adjusted padding for social links * fixed close button padding
PR Summary:
Links, Social Icons, Language, Country/currency, and Account icon/label are aligned with close button and cart icon.
Why are these changes introduced?
Fixes #2671
What approach did you take?
Visual impact on existing themes
Testing steps/scenarios
Demo links
Checklist