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

Add mobile/tablet grid settings #1352

Merged
merged 50 commits into from
Feb 25, 2022
Merged

Add mobile/tablet grid settings #1352

merged 50 commits into from
Feb 25, 2022

Conversation

sofiamatulis
Copy link
Contributor

@sofiamatulis sofiamatulis commented Feb 11, 2022 •
edited
Loading

Why are these changes introduced?

Adds grid settings for mobile and tablet. The options are only 1 or 2 columns for both. Because we have been using the mobile language in the labels even when affecting tablet, we are keeping the language consistent.

Because there are only two options, we can't use the range, so we are using the select dropdown instead.

What approach did you take?

I am using the existing tablet classes and adding a new setting columns_mobile to dictate how many columns we will need on tablet/mobile

Other considerations

Testing steps/scenarios

  • Test all of these sections on mobile/tablet and with 1 and 2 columns selected
    • Featured Collection section
    • Collection list section
    • Product recs
    • Search Results page
    • Multicolumn section
    • Blog section
    • Collection page
  • Test in combination with global settings
  • Test with enable swipe on mobile on

Demo links
Please include a link to a demo store that includes preconfigured sections and settings to allow reviewers to easily test the features you are working on.

Checklist

sofiamatulis
sofiamatulis commented Feb 11, 2022
View reviewed changes
kmeleta
kmeleta reviewed Feb 11, 2022
View reviewed changes
Copy link
Contributor

@kmeleta kmeleta left a comment

Choose a reason for hiding this comment

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

I'm leaving a couple quick comments about the grid naming/media query patterns. Mostly to outline my thinking and see if anyone else has strong feelings. Might sleep on it.

@melissaperreault melissaperreault removed their request for review February 11, 2022 19:38
ludoboludo
ludoboludo reviewed Feb 11, 2022
View reviewed changes
Copy link
Contributor

@ludoboludo ludoboludo left a comment

Choose a reason for hiding this comment

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

Leaving a few comments, it doesn't seem to work everywhere as expected.

@katycobb
Copy link
Contributor

katycobb commented Feb 12, 2022 •
edited by sofiamatulis
Loading

Hey Sofia, I think these sections should have a MOBILE LAYOUT heading added since we’re adding mobile-specific settings. Here’s what I’d suggest:

Featured Collection section

  • Add MOBILE LAYOUT heading above SECTION PADDING. put Number of columns on mobile setting first, Enable swipe on mobile below it.

Collection list section

  • Add MOBILE LAYOUT heading above SECTION PADDING. put Number of columns on mobile setting first, Enable swipe on mobile below it.

Product recs

  • Add MOBILE LAYOUT heading above SECTION PADDING. put Number of columns on mobile setting below heading

Search Results page

  • Add MOBILE LAYOUT heading above SECTION PADDING. put Number of columns on mobile setting below heading

Multicolumn section

  • Add MOBILE LAYOUT heading above SECTION PADDING. put Number of columns on mobile setting first, Enable swipe on mobile below it.

Blog section

  • Add MOBILE LAYOUT heading above SECTION PADDING. put Number of columns on mobile setting below heading
  • Note: changing this setting between 1 and 2 columns doesn’t seem to do anything. (looks like @ludoboludo noted this too)

Collection page

  • Add MOBILE LAYOUT heading above SECTION PADDING. put Number of columns on mobile setting below heading

@ghost ghost added the cla-needed label Feb 14, 2022
@sofiamatulis
Copy link
Contributor Author

sofiamatulis commented Feb 14, 2022 •
edited
Loading

Hey Sofia, I think these sections should have a MOBILE LAYOUT heading added since we’re adding mobile-specific settings. Here’s what I’d suggest:

@katycobb All content / order has been updated :) Let me know if it looks good and I will request translations

@sofiamatulis sofiamatulis mentioned this pull request Feb 14, 2022
Closed
@katycobb
Copy link
Contributor

Hey Sofia, I think these sections should have a MOBILE LAYOUT heading added since we’re adding mobile-specific settings. Here’s what I’d suggest:

@katycobb All content / order has been updated :) Let me know if it looks good and I will request translations

Looks good, thanks!

kmeleta
kmeleta reviewed Feb 15, 2022
View reviewed changes
Copy link
Contributor

@kmeleta kmeleta left a comment

Choose a reason for hiding this comment

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

Couple notes on the code. Happy to huddle if I'm being verbose and confusing 😅

@sofiamatulis
Copy link
Contributor Author

sofiamatulis commented Feb 16, 2022 •
edited
Loading

I noticed that besides the Multicolumn section, all of the other sections when we have enable swipe on mobile affect mobile and tablet. However, Multicolumn only affects mobile

Why is this weird?
It's inconsistent with other sections . Also when we use the language mobile for labels and it affects mobile and tablet but have this one section only affect mobile (slider setting), it's a weird experience
https://screenshot.click/16-02-fxmf7-8mt42.png

In this video, you will see that featured collection has slider on tablet and mobile but multicolumn doesn't (only mobile):

https://screenshot.click/16-41-vp7zq-l6ytf.mp4

I dont think this should be changed on this PR but in a follow up since it's existing on main and something that wasnt introduced here and will need some testing. But it's something definitely to consider 🤔 Not sure if there was a reason why the slider was done this way

cc @katycobb @wiktoriaswiecicka @nicklepine

kmeleta
kmeleta reviewed Feb 17, 2022
View reviewed changes
Copy link
Contributor

@kmeleta kmeleta left a comment

Choose a reason for hiding this comment

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

Did a brief check of the affected sections and it seems like things are working well. Couple more code comments though.

@sofiamatulis sofiamatulis requested a review from kmeleta February 17, 2022 21:27
kmeleta
kmeleta reviewed Feb 23, 2022
View reviewed changes
Copy link
Contributor

@kmeleta kmeleta left a comment

Choose a reason for hiding this comment

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

Couple more small code comments

@sofiamatulis sofiamatulis requested a review from kmeleta February 23, 2022 16:03
kmeleta
kmeleta reviewed Feb 23, 2022
View reviewed changes
Sofia Matulis added 2 commits February 23, 2022 14:42
@sofiamatulis sofiamatulis requested a review from kmeleta February 23, 2022 19:44
ludoboludo
ludoboludo reviewed Feb 23, 2022
View reviewed changes
This was referenced Feb 23, 2022
Open
Open
ludoboludo
ludoboludo reviewed Feb 24, 2022
View reviewed changes
Copy link
Contributor

@ludoboludo ludoboludo left a comment

Choose a reason for hiding this comment

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

I think other than what I added as comment, it seem pretty good and almost ready to go 👍

ludoboludo
ludoboludo approved these changes Feb 24, 2022
View reviewed changes
Copy link
Contributor

@ludoboludo ludoboludo left a comment

Choose a reason for hiding this comment

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

Damn 1 columns does look big on tablet 🤔

But the rest looks good, I think it's shippable! 🎉

kmeleta
kmeleta approved these changes Feb 25, 2022
View reviewed changes
Copy link
Contributor

@kmeleta kmeleta left a comment

Choose a reason for hiding this comment

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

mmm I may agree with Ludo. Maybe we can discuss it again with UX at some point. For now though :shipit:

@sofiamatulis sofiamatulis merged commit 406cb6c into main Feb 25, 2022
@sofiamatulis sofiamatulis deleted the grid-mobile branch February 25, 2022 15:55
This was referenced Feb 25, 2022
Closed
Open
@melissaperreault
Copy link
Contributor

I am currently upgrading our demo store to the newest version and noticed this too; I agree do believe the 1 column on Tablet is way too much. I'll follow up with next steps for this one! 🙏

phapsidesGT pushed a commit to Gravytrain-UK/gt-shopify-dawn-theme that referenced this pull request Sep 3, 2024
@sofiamatulis
Add mobile/tablet grid settings (Shopify#1352)
8ffdca1
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@melissaperreault melissaperreault melissaperreault left review comments

@bredowmax bredowmax bredowmax left review comments

@ludoboludo ludoboludo ludoboludo approved these changes

@kmeleta kmeleta kmeleta approved these changes

@wiktoriaswiecicka wiktoriaswiecicka wiktoriaswiecicka left review comments

@katycobb katycobb Awaiting requested review from katycobb

@nicklepine nicklepine Awaiting requested review from nicklepine

Assignees

@ludoboludo ludoboludo

@kmeleta kmeleta

Labels
cla-needed
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
8 participants
@sofiamatulis @katycobb @melissaperreault @nicklepine @ludoboludo @bredowmax @kmeleta @wiktoriaswiecicka