-
Notifications
You must be signed in to change notification settings - Fork 3.6k
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
[Theme Settings] Re-order Brand Information settings and add help text #2256
Conversation
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.
This looks good to me.
I did have some feedback (non-blocking) regarding the impact of these changes in relation to theme releases + our other 1p themes (see below).
I'll let you make the call on whether this should be handled in this PR or not.
@@ -1397,6 +1366,41 @@ | |||
} | |||
] | |||
}, | |||
{ | |||
"name": "t:settings_schema.brand_information.name", |
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.
(not related to this line of code)
Context
Currently, all of our 1p themes contain these brand settings in their respective settings_data.json
-- here's Craft:
{
"current": "Default",
"presets": {
"Default": {
"logo_width": 100,
"brand_headline": "",
"brand_description": "<p></p>",
"brand_image_width": 100,
https://github.com/Shopify/craft/blob/main/config/settings_data.json#L6-L8
Questions
Looking at the current version of main
on dawn:
https://github.com/Shopify/dawn/blob/main/config/settings_data.json
- Should we also add
"brand_headline": "",
,"brand_description": "<p></p>",
,"brand_image_width": 100,
toconfig/settings_data.json
of dawn? - And should we also reposition these settings below
"accent_icons": "text",
? (see video below)
Here's a video showing how we would do this using Craft for example (it'd be the same for all other 1p themes):
Reposition.brand.settings.in.settings.schema.mp4
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.
Good point. I'm not 100% sure if we should add these defaults on this repo specifically (because of conflicts - did we do that for other settings recently?), but yes, ideally all our themes should have them. Also, I think we shouldn't have "brand_description": "<p></p>"
as default, otherwise the setting will not be empty.
As for the order, it's not a big deal but makes it more organized. I would move them to above social links, yes :D
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.
Either way, this could be in its own separate PR as we prepare the releases.
PR Summary:
Moves
Brand information
settings aboveSocial icons
on Theme Settings.Add help text to brand information.
Why are these changes introduced?
Providing more clarity for where Brand Information settings are used.
Demo links
Checklist