Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
Feat. collection updates #1295
Feat. collection updates #1295
Changes from 26 commits
13b2b2b
e13619e
2b9742e
7256df0
a5c1c1d
8cc4740
55f9b3c
43c969c
a3e4559
46e1569
aaefcb5
a4589ab
f3b2ccf
b204e36
19e42e5
de421ec
8154155
881dd6b
35015be
7123217
4d34092
1aac0bc
a6a3ea0
8c94a58
88f5165
f4a4b0d
d4f48c1
9cfbc2c
2b96ef8
62ba6c2
0f51041
39050ce
9b6c6b5
f816e38
521f312
1cd72e1
221287e
e31b11f
11e5424
a1f2824
219ee7e
98f1e41
e4c2a27
24ecfba
50ba6ec
1e1b287
078276a
32f6cd4
b54d81f
2229142
8a43e00
81c2759
b72f770
3808adc
b044959
c083fa6
fcc3c6e
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
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.
Had to make this a desktop only thing when the slider is enabled.
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.
@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!
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.
@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?
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.
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
, andUse 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 beButton
instead since the carousel if set tofalse
by default on desktop? cc. @YoannJailin @danielvanThere 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.
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 beCall 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.
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 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.
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.
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.
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.
@YoannJailin Yes, the goal would be to offer all 3 button styles everywhere we show a button for predictability and consistency 👌