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

[Theme Settings] Re-order Brand Information settings and add help text #2256

Merged
merged 2 commits into from
Jan 27, 2023

Conversation

LucasLacerdaUX
Copy link
Contributor

@LucasLacerdaUX LucasLacerdaUX commented Jan 25, 2023

PR Summary:

Moves Brand information settings above Social 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

@LucasLacerdaUX LucasLacerdaUX changed the title [Brand information] Re-order and add help text [Brand Information] Re-order and add help text Jan 25, 2023
@LucasLacerdaUX LucasLacerdaUX changed the title [Brand Information] Re-order and add help text [Theme Settings] Re-order Brand Information settings and add help text Jan 25, 2023
@andrewetchen andrewetchen self-requested a review January 27, 2023 14:33
@ludoboludo ludoboludo self-requested a review January 27, 2023 15:09
Copy link
Contributor

@andrewetchen andrewetchen left a 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",
Copy link
Contributor

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

  1. Should we also add "brand_headline": "",, "brand_description": "<p></p>",, "brand_image_width": 100, to config/settings_data.json of dawn?
  2. 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

Copy link
Contributor Author

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

Copy link
Contributor Author

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.

@LucasLacerdaUX LucasLacerdaUX merged commit 788f19b into main Jan 27, 2023
@LucasLacerdaUX LucasLacerdaUX deleted the help-text-brand branch January 27, 2023 18:11
phapsidesGT pushed a commit to Gravytrain-UK/gt-shopify-dawn-theme that referenced this pull request Sep 3, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants