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.

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

Add grid settings (number of columns) #989

Merged
merged 39 commits into from
Feb 10, 2022
Merged

Add grid settings (number of columns) #989

merged 39 commits into from
Feb 10, 2022

Conversation

sofiamatulis
Copy link
Contributor

@sofiamatulis sofiamatulis commented Dec 8, 2021

Why are these changes introduced?

Fixes #988 .

What approach did you take?

I have added a new setting for all these sections:

https://screenshot.click/25-24-udz1y-6h1gp.png

We have also removed the magic making the elements fill the width of the row: https://screenshot.click/25-48-9vyjp-dfgfx.png (adaptive grid on smaller numbers)

  • Featured Collection section
  • Collection list section
  • Product recs
  • Search Results page
  • Multicolumn section
  • Blog section
  • Collection page

Other considerations

We have allowed only the multicolumn to have a max of 6 (logos scenario). All of the other sections have a max of 5

Featured collection range for existing setting products to show is now incrementing by 1 instead of 2

Demo links

Ensure this works well with the newly added settings:

  • New Page Width Range [1000-1600]
  • Global settings [ borders, spacing, shadows]

Checklist

@sofiamatulis sofiamatulis changed the title Grid Add grid settings (number of columns) Dec 8, 2021
@sofiamatulis sofiamatulis marked this pull request as ready for review January 25, 2022 15:23
@ghost ghost added the cla-needed label Jan 25, 2022
@katycobb
Copy link
Contributor

katycobb commented Jan 26, 2022

Hey team, just getting caught up on this + refreshing my memory on the design work we did which feels like it was YEARS ago 😅

Agree with Nick's suggestion that communicating this as a maximum is ideal to communicate how it'll behave.

@wiktoriaswiecicka it looks like we'd done work to add this to the Settings UI Figma file, but we had separate values for desktop and mobile. Seems like we'll only have one setting, and mobile will auto adjust. Does that sound right?

Edited to add: in the new featured collection section, we have designed for "Number of columns on desktop" and "Number of columns on mobile" Wik we should align on approach with Yoann.

@sofiamatulis
Copy link
Contributor Author

Hey team, just getting caught up on this + refreshing my memory on the design work we did which feels like it was YEARS ago 😅

Agree with Nick's suggestion that communicating this as a maximum is ideal to communicate how it'll behave.

@wiktoriaswiecicka it looks like we'd done work to add this to the Settings UI Figma file, but we had separate values for desktop and mobile. Seems like we'll only have one setting, and mobile will auto adjust. Does that sound right?

Edited to add: in the new featured collection section, we have designed for "Number of columns on desktop" and "Number of columns on mobile" Wik we should align on approach with Yoann.

👋 @katycobb, for some context we had decided to only add this for desktop as mobile would be too squished to have 3+ rows 🤔

@nicklepine
Copy link

@sofiamatulis Alright I suggest you remove the "magic" that has items occupy the full width altogether (i.e. this https://screenshot.click/27-38-1ztno-n5kai.png). This means this could happen if there are less items that the number entered in the setting (https://screenshot.click/25-16-788jk-7eezr.png) (i.e. only 3 items, but we allow 6 per row).

We'll play with it and decide if this feels correct. It's hard to make decisions without actually interacting with what the merchant would experience.

@katycobb
Copy link
Contributor

👋 @katycobb, for some context we had decided to only add this for desktop as mobile would be too squished to have 3+ rows 🤔

On the new featured collection section design, the mobile grid looks like this. I think the UXers need to align on approach. CC: @wiktoriaswiecicka and @YoannJailin
27-25-lsqco-fpn5l

@sofiamatulis
Copy link
Contributor Author

sofiamatulis commented Jan 27, 2022

@sofiamatulis Alright I suggest you remove the "magic" that has items occupy the full width altogether (i.e. this https://screenshot.click/27-38-1ztno-n5kai.png). This means this could happen if there are less items that the number entered in the setting (https://screenshot.click/25-16-788jk-7eezr.png) (i.e. only 3 items, but we allow 6 per row).

We'll play with it and decide if this feels correct. It's hard to make decisions without actually interacting with what the merchant would experience.

@nicklepine I have updated it so you can play with the test store and see what that would feel like. Quick video: https://screenshot.click/27-23-143ql-5izvn.mp4

It feels a bit more intuitive to me since the merchant can update the row numbers per section and if there is more/less items they can update the number of rows. It ensures it's all consistent since all rows behave the same and it behaves exactly like what the setting is saying it's doing

@nicklepine
Copy link

nicklepine commented Jan 28, 2022

Thank you! @sofiamatulis for spinning this up!

Internal Link to Recording

Recap:

  • After testing, I'm aligned to remove any magical pattern because both settings now indicate the rule to follow
  • Improve empty states related to the settings range so merchants have better instant view of what they are picking (@wiktoriaswiecicka can likely pair with you)
  • I encourage @katycobb to give it a go too at testing in case you want to revisit any of the setting naming based on what they impact.

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'm curious about some of the behaviour specifically on tablet/mobile. We want to remove the magic effect but we no specific logic that is self explanatory for the amount of items per row on tablet/mobile.
You'd have to test with the different numbers of products per row to see what the behaviour is like on smaller screens.

One other thing we're going to need to edit, is the sizes and srcset attribute on medias in some place as now there can be one per row. And we did not account for this before.

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.

@ludoboludo found and mentioned the biggest issues/questions that I have right now.

I'm curious about some of the behaviour specifically on tablet/mobile. We want to remove the magic effect but we no specific logic that is self explanatory for the amount of items per row on tablet/mobile.

I personally think it's fine that the number of items per row on tablet is still arbitrarily set to whatever we think works best (product grids 3, multicolumn 2, product reccs 2). I think that's less magic and more predictable than something like if desktop > 4 then tablet = 3 , if desktop <= 4 then tablet = 2. And I think adding a tablet specific setting is bad idea.

Otherwise I just want to ensure we enforce the strict widths on tablet as we do on desktop now. I missed much of the conversation around removing/keeping this sort of magic, but I do agree with the ultimate decision. I think it makes sense and the code will also be easier to build on in the future.

@katycobb
Copy link
Contributor

katycobb commented Feb 4, 2022

Hi team,

UX folks connected + agreed we should add mobile settings back, but with a smaller slider range of 1-2. @wiktoriaswiecicka can advise which sections should get this. It should be slotted under the Mobile Layout section of settings. Note: if there are any sections that don't get the mobile setting, we'll want to add help text explaining that it's automatically optimized for mobile.

I'd like to standardize all the labels to one common label for each viewport, rather than calling out products, blog posts, items, etc. differently for each section:

Number of columns on desktop
Number of columns on mobile 

I think these labels make sense given the magic removal - we don't need to specify "maximum" because it should be an absolute number, but open to the team's feedback!

In addition to the proposed label updates, I have a suggestions for hierarchy changes:

Featured collection
-move setting to below Maximum products to show

Collection list
-Move to below Image ratio

Product recs
-move to below Heading

Search results
-Move to above the PRODUCT CARD section since this will affect blog cards too.

Multicolumn
-Placement is good

Blog posts
-Is the setting duplicated here? Or is the existing Blog posts slider controlling the max number to show, and the new setting will control the number of columns?

If it should just be one setting to control the grid, I’d suggest: Update existing label Blog posts to Number of columns on desktop and remove Number of blogs in a row on desktop
Let me know if this isn't the case + I can revisit.

Collection page
-Placement is good

Happy to chat through anything if it doesn't make sense or if you have questions/ideas!

@sofiamatulis
Copy link
Contributor Author

Update:

This PR only accounts for the desktop settings. The mobile settings will be done in a follow up

@wiktoriaswiecicka
Copy link

In multicolumn the first two columns are shifted higher to the grid on desktop:

Screen Shot 2022-02-07 at 10 30 29 AM

and this is how it looks with 6 columns 🥴
Screen Shot 2022-02-07 at 10 40 50 AM

@sofiamatulis
Copy link
Contributor Author

sofiamatulis commented Feb 7, 2022

In multicolumn the first two columns are shifted higher to the grid on desktop:

@wiktoriaswiecicka This was a previous style :) should be fixed now

@sofiamatulis
Copy link
Contributor Author

Number of columns on desktop

Hi team,

UX folks connected + agreed we should add mobile settings back, but with a smaller slider range of 1-2. @wiktoriaswiecicka can advise which sections should get this. It should be slotted under the Mobile Layout section of settings. Note: if there are any sections that don't get the mobile setting, we'll want to add help text explaining that it's automatically optimized for mobile.

I'd like to standardize all the labels to one common label for each viewport, rather than calling out products, blog posts, items, etc. differently for each section:

Number of columns on desktop
Number of columns on mobile

@katycobb Since we already have Number of products/blogs etc. Wondering if we name it Number of columns in a row on desktop. So it is specific to the number of items in a specific row and not overall the number of total columns

@sofiamatulis
Copy link
Contributor Author

Blog posts -Is the setting duplicated here? Or is the existing Blog posts slider controlling the max number to show, and the new setting will control the number of columns?

If it should just be one setting to control the grid, I’d suggest: Update existing label Blog posts to Number of columns on desktop and remove Number of blogs in a row on desktop Let me know if this isn't the case + I can revisit.

@katycobb We have 2 settings here. One to show the total amount of blog posts (existing setting) and the new one to show the amount of columns per row.

@sofiamatulis sofiamatulis requested a review from kmeleta February 8, 2022 19:29
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.

Looks good 👌

Just left a small nitpick about whitespace :)

Co-authored-by: Ludo <ludo.segura@shopify.com>
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.

One thing that should be a quick fix.

Otherwise, I think I gave it a pretty good look and things seem like they're behaving as expected. I think there's some more cleanup to do with the classes on these sections, but there's not much we can/should do in that regard until we're also considering the mobile/tablet concerns.

@sofiamatulis sofiamatulis requested a review from kmeleta February 10, 2022 16:41
@sofiamatulis sofiamatulis requested a review from kmeleta February 10, 2022 17:04
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.

Thanks for the updates! Looks solid to me 👍

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.

🎉

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add grid settings (number of columns)
6 participants