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

Feat. collection updates #1295

Merged
merged 57 commits into from
Mar 11, 2022
Merged

Feat. collection updates #1295

Show file tree
Hide file tree
Changes from 14 commits
Commits
Show all changes
57 commits
Select commit Hold shift + click to select a range
13b2b2b
Feat. collection updates
ludoboludo Jan 31, 2022
e13619e
add uppercase option
ludoboludo Jan 31, 2022
2b9742e
updates to add slider option on desktop
ludoboludo Feb 1, 2022
7256df0
change data step to 1 for the time being
ludoboludo Feb 1, 2022
a5c1c1d
remove existing swipe settings
ludoboludo Feb 2, 2022
8cc4740
change setting order, limit the horizontal offset
ludoboludo Feb 3, 2022
55f9b3c
add period
ludoboludo Feb 3, 2022
43c969c
remove peek and scroll snap padding
ludoboludo Feb 3, 2022
a3e4559
move description chekbox around and adjust copy
ludoboludo Feb 4, 2022
46e1569
add checkboxes to enable carousel on desktop and mobile
ludoboludo Feb 4, 2022
aaefcb5
use a new slider--desktop class
ludoboludo Feb 7, 2022
a4589ab
Merge branch 'main' into feat-collection
ludoboludo Feb 7, 2022
f3b2ccf
remove heading class and add a11y attributes
ludoboludo Feb 7, 2022
b204e36
re add grid peek behaviour at all time on mobile
ludoboludo Feb 7, 2022
19e42e5
fix media query and remove unnecessary styling
ludoboludo Feb 8, 2022
de421ec
edit copy
ludoboludo Feb 8, 2022
8154155
start exploring grid peek on desktop
ludoboludo Feb 9, 2022
881dd6b
add grid peek by default on desktop
ludoboludo Feb 10, 2022
35015be
fix page width depending on scenarios and adjust slide width
ludoboludo Feb 10, 2022
7123217
fix slider disabled button on wide screens. Remove settings added in …
ludoboludo Feb 11, 2022
4d34092
Merge branch 'main' into feat-collection
ludoboludo Feb 11, 2022
1aac0bc
change the calculation for item width based on the number of items pe…
ludoboludo Feb 11, 2022
a6a3ea0
change total calculation for slide counter
ludoboludo Feb 15, 2022
8c94a58
add full width setting
ludoboludo Feb 16, 2022
88f5165
Merge branch 'main' into feat-collection
ludoboludo Feb 16, 2022
f4a4b0d
fix full width CSS styling specificity
ludoboludo Feb 16, 2022
d4f48c1
fix pages calculation to work on desktop
ludoboludo Feb 17, 2022
9cfbc2c
address some of the UX feedback
ludoboludo Feb 18, 2022
2b96ef8
add spacing for shadows
ludoboludo Feb 18, 2022
62ba6c2
address reviewer's comments
ludoboludo Feb 21, 2022
0f51041
Edit copy
ludoboludo Feb 22, 2022
39050ce
edit copy
ludoboludo Feb 23, 2022
9b6c6b5
Update 21 translation files
translation-platform[bot] Feb 23, 2022
f816e38
Update 1 translation file
translation-platform[bot] Feb 23, 2022
521f312
Update 13 translation files
translation-platform[bot] Feb 23, 2022
1cd72e1
Update 1 translation file
translation-platform[bot] Feb 24, 2022
221287e
Update 6 translation files
translation-platform[bot] Feb 24, 2022
e31b11f
Update 2 translation files
translation-platform[bot] Feb 24, 2022
11e5424
Update 19 translation files
translation-platform[bot] Feb 24, 2022
a1f2824
Merge branch 'main' into feat-collection
ludoboludo Feb 25, 2022
219ee7e
fix translation comma
ludoboludo Feb 25, 2022
98f1e41
Merge branch 'main' into feat-collection
ludoboludo Mar 1, 2022
e4c2a27
Merge branch 'main' into feat-collection
ludoboludo Mar 2, 2022
24ecfba
reuse assign to hide slider controls
ludoboludo Mar 2, 2022
50ba6ec
move styling to be more general
ludoboludo Mar 2, 2022
1e1b287
fix UX feedback
ludoboludo Mar 4, 2022
078276a
Address Lucas' feedback
ludoboludo Mar 7, 2022
32f6cd4
reorder setting
ludoboludo Mar 7, 2022
b54d81f
remove unused custom properties
ludoboludo Mar 7, 2022
2229142
fix class specificity
ludoboludo Mar 8, 2022
8a43e00
remove redundant page-width-mobile class
ludoboludo Mar 8, 2022
81c2759
address reviewer's comment
ludoboludo Mar 8, 2022
b72f770
apply reduced motion media query to all sliders
ludoboludo Mar 8, 2022
3808adc
remove some CSS
ludoboludo Mar 9, 2022
b044959
fix UX feedback
ludoboludo Mar 10, 2022
c083fa6
tweak title spacing
ludoboludo Mar 10, 2022
fcc3c6e
fix spacing
ludoboludo Mar 11, 2022
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
27 changes: 25 additions & 2 deletions assets/component-slider.css
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -99,7 +99,7 @@ slider-component {
}

@media (prefers-reduced-motion) {
.slider--everywhere {
.slider--everywhere, .slider--desktop {
ludoboludo marked this conversation as resolved.
Show resolved Hide resolved
scroll-behavior: auto;
}
}
Expand All @@ -109,6 +109,29 @@ slider-component {
scroll-snap-align: center;
}

@media screen and (max-width: 989px) {
.slider--desktop:not(.slider--tablet) + .slider-buttons {
display: none;
}
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Had to make this a desktop only thing when the slider is enabled.

}

@media screen and (min-width: 990px) {
.slider--desktop {
position: relative;
flex-wrap: inherit;
overflow-x: auto;
scroll-snap-type: x mandatory;
ludoboludo marked this conversation as resolved.
Show resolved Hide resolved
scroll-behavior: smooth;
-webkit-overflow-scrolling: touch;
margin-bottom: 1rem;
}

.slider.slider--everywhere .slider__slide {
margin-bottom: 0;
padding-bottom: 0;
}
}

/* Scrollbar */

.slider {
Expand Down Expand Up @@ -221,7 +244,7 @@ slider-component {
}

@media screen and (min-width: 990px) {
.slider:not(.slider--everywhere) + .slider-buttons {
.slider:not(.slider--everywhere):not(.slider--desktop) + .slider-buttons {
display: none;
}
}
Expand Down
12 changes: 12 additions & 0 deletions assets/template-collection.css
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -44,6 +44,18 @@
margin-bottom: 15rem;
}

@media screen and (max-width: 989px) {
.collection .slider--tablet.product-grid {
scroll-padding-left: 1.5rem;
}
}

@media screen and (max-width: 989px) {
.collection .slider--desktop.product-grid {
scroll-padding-left: 0;
}
}

.collection__view-all {
margin-top: 2rem;
}
3 changes: 2 additions & 1 deletion locales/en.default.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -261,7 +261,8 @@
},
"featured_collection": {
"view_all": "View all",
"view_all_label": "View all products in the {{ collection_name }} collection"
"view_all_label": "View all products in the {{ collection_name }} collection",
"slider": "Slider"
},
"collection_list": {
"view_all": "View all"
Expand Down
80 changes: 78 additions & 2 deletions locales/en.default.schema.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -578,6 +578,55 @@
"title": {
"label": "Heading"
},
"title_size": {
"label": "Heading size",
"options__1":{
"label": "Small"
},
"options__2":{
"label": "Medium"
},
"options__3":{
"label": "Large"
}
},
"horizontal_offset": {
"label": "Heading horizontal offset"
},
"vertical_offset": {
"label": "Heading vertical offset",
"info": "Offset will only apply when no collection description is shown."
},
"description": {
"label": "Description"
},
"show_description": {
"label": "Show collection description from the admin"
},
"description_style": {
"label": "Description style",
"options__1": {
"label": "Body"
},
"options__2": {
"label": "Subtitle"
},
"options__3": {
"label": "Uppercase"
}
},
"heading_alignment": {
"label": "Heading and description alignment",
"options__1": {
"label": "Left"
},
"options__2": {
"label": "Center"
},
"options__3": {
"label": "Right"
}
},
"collection": {
"label": "Collection"
},
Expand All @@ -587,8 +636,29 @@
"show_view_all": {
"label": "Enable \"View all\" button if collection has more products than shown"
},
"swipe_on_mobile": {
"label": "Enable swipe on mobile"
"view_all_style": {
"label": "View all style",
Copy link
Contributor

Choose a reason for hiding this comment

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

@katycobb Sharing an observation here, I was wondering how we feel about having "View all" style between quotation mark in the info text but not in the style label?

Further, I was reflecting about that we are still talking of a button's aesthetic outcome here, what makes this setting a less generic candidate to refer the label as oppose to the component? Asking to hear your thoughts!

Copy link

Choose a reason for hiding this comment

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

@melissaperreault I'd agree "View all" in the label should not have quotation marks - I think this exists in a few other sections - I'll need to document and get them updated if we change here.

If I understand your second question correctly, I opted to remove the mention of button because one of the styling options will be a link, rather than a button. Does that make sense?

Copy link
Contributor

Choose a reason for hiding this comment

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

If we want to update the label I suggest changing it all at once in another issue if we decide to remove the quotation marks. I recommend making sure that this change would translate well to other languages.

For the second part, this is what I assumed. I find the label don't match other more neutral style settings. It's hard to quickly spot while scanning the list of settings where the button style is. I don't think this would translate well to other languages either.

i.e. Other neutral form: We have already **Text** style, **Description** style, and Use button outline style; **View all** style stood out to me.

Also, this section is the only one to wear the link button style by default, when a merchant adds the section among other section, its default button visual weight would be different, should it be Button instead since the carousel if set to false by default on desktop? cc. @YoannJailin @danielvan

Copy link

Choose a reason for hiding this comment

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

Hm I see what you mean with the label, Meli. Do you think it would be better to say Button style even if one of the options is a link? Maybe a more neutral approach could be Call to action style. I'll do some exploration, thanks for flagging.

RE: default, if all other sections use button as default I agree that should be the case here too. Will be easier for merchants to achieve a consistent look.

Copy link
Contributor

Choose a reason for hiding this comment

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

I leave it to @YoannJailin on the button question. By default, is the experience better or more consistent if the default style is button? If so, then I'd say we should do button.

Copy link

Choose a reason for hiding this comment

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

Hello! About the view all option, since Featured Collection is currently the only section where we can display the view all option as a link, i would keep the default one as a button for now. @melissaperreault @danielvan @katycobb @ludoboludo
It makes me realize that it should be consistant with the Blog Posts section. Maybe we also should add the styling option in that section.

Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe we also should add the styling option in that section.

@YoannJailin Yes, the goal would be to offer all 3 button styles everywhere we show a button for predictability and consistency 👌

"options__1": {
"label": "Link"
},
"options__2": {
"label": "Outline button"
},
"options__3": {
"label": "Solid button"
}
},
"enable_desktop_slider": {
"label": "Enable carousel on desktop"
},
"carousel_style": {
"label": "Carousel style",
"options__1": {
"label": "Full cards"
},
"options__2": {
"label": "Cards with peek"
}
},
"header": {
"content": "Product card"
Expand All @@ -614,6 +684,12 @@
"show_rating": {
"label": "Show product rating",
"info": "To display a rating, add a product rating app. [Learn more](https://help.shopify.com/manual/online-store/themes/os20/themes-by-shopify/dawn/sections#featured-collection-show-product-rating)"
},
"mobile_header": {
"content": "Mobile Layout"
},
"enable_mobile_slider": {
"label": "Enable carousel on tablet/mobile"
ludoboludo marked this conversation as resolved.
Show resolved Hide resolved
}
},
"presets": {
Expand Down
Loading