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

Jump to bottom

[bug] Horizontal scrolling on mobile. #2617

Merged
merged 2 commits into from
May 10, 2023
Merged

[bug] Horizontal scrolling on mobile. #2617

Changes from 1 commit
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
4305882
Add overflow-x:hidden to prevent horizontal scrolling on mobile.
eugenekasimov May 9, 2023
19f1961
Change margin for slider for mobile and tablet.
eugenekasimov May 9, 2023
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
1 change: 1 addition & 0 deletions assets/base.css
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2110,6 +2110,7 @@ product-info .loading-overlay:not(.hidden) ~ *,
/* utility-bar */
.utility-bar {
height: 100%;
overflow-x: hidden;
Copy link
Contributor

Choose a reason for hiding this comment

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

I wonder if this is the best way to go about it. I think there is a also another way for mobile to deal with it 🤔 Here we might create this fix when maybe we should revisit our spacing approach ?

We're using negative margin, -1.6rem, for the slider buttons that isn't matching the mobile padding 1.5rem.

I think this could be a more logical fix 🤔 If in fact it does also solve our issue.

Copy link
Contributor Author

@eugenekasimov eugenekasimov May 9, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

Thanks Ludo, I think it's more logical and right solution from technical perspective, however it takes minor visual changes.

I also can assume that we have the same issue for tablet devices, unfortunately I don't have a real tablet to test it, but I would change margin for tablet from -3.8rem to -3.2rem.

All of these lead us to a small visual change but it's really minor.

@melissaperreault @YoannJailin Could you please confirm that we are okay to make those changes?

Before Screenshot 2023-05-09 at 9 58 26 AM Screenshot 2023-05-09 at 9 58 34 AM
After Screenshot 2023-05-09 at 9 57 37 AM Screenshot 2023-05-09 at 9 57 47 AM

Copy link
Contributor

Choose a reason for hiding this comment

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

I think you can just change the 1.6 value to 1.5 and leave the rest untouched 🤔

We don't have the same issue for tablet devices 👍
On tablet sizes, over 750px and under 990px, the padding on the sides is 5rem while the negative margin applied to the buttons is -3.8rem so we should be safe here 🎉

Copy link
Contributor Author

@eugenekasimov eugenekasimov May 9, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

Oh, you are right, I was actually looking at the header where paddings are 3.2rem.

But anyway, don't you think that, after we change 1.6 to 1.5, 3.2 rem looks a bit better when you try to see how the buttons and hamburger icons are aligned?

If don't change margin for tablet it looks like this 👇
Screenshot 2023-05-09 at 11 40 37 AM

And if we go for changing 3.8 to 3.2 for tablet it looks like this 👇
Screenshot 2023-05-09 at 11 40 00 AM

which is more closer to what it looks on mobile 👇
Screenshot 2023-05-09 at 11 40 13 AM

@ludoboludo @melissaperreault @YoannJailin

Copy link
Contributor

Choose a reason for hiding this comment

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

Yeah I don't think it's much of an issue for me to keep the tablet look how it currently is. I think that alignment is ideally what we would have on mobile actually. But since the svg icon is a different size compared to the hamburger menu icon it makes it a bit trickier to align.

There is also the possibility to add some extra negative margin to the icon. So the 0.1rem we removed by going from 1.6 -> 1.5 can be added to .slider-button--prev .icon through margin-left: -0.1rem;

Though I don't really think we need to go to that level of nitpicking. Curious to see what designers think 👀

Copy link
Contributor

Choose a reason for hiding this comment

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

I am okay with the alignment proposed! 👍

Copy link
Contributor

@melissaperreault melissaperreault May 10, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

The alignment on what is live currently on the PR demo link works for me. We could be nitpicking for a long time. Someone could argue that the visual impression of the alignment feels right even though it's not when you trace a line to the edge of the icon. The live version of this PR feels good for now, both for Tablet and Mobile. 👍

}

.utility-bar--bottom-border {
Expand Down