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

Update Preset default #410

Merged
merged 10 commits into from
Aug 18, 2021
Merged

Update Preset default #410

merged 10 commits into from
Aug 18, 2021

Conversation

ludoboludo
Copy link
Contributor

@ludoboludo ludoboludo commented Aug 16, 2021

Why are these changes introduced?

Fixes #58 .

What approach did you take?

Updated the default values based on the linked issue.

Other considerations

Demo links

Checklist

@ludoboludo ludoboludo marked this pull request as draft August 16, 2021 20:43
@ludoboludo ludoboludo marked this pull request as ready for review August 16, 2021 21:01
@martinamarien martinamarien self-requested a review August 17, 2021 13:27
@martinamarien
Copy link
Contributor

Colors look good based on the preset values Wik provided.

I'd like us to touch base with @Guillaumegranger1 and/or @wiktoriaswiecicka about the announcement bar, header and footer preset settings. Because these are static sections, it means that their settings are going to be stored in the settings_data.json file and not in a template.json file. This means, we can provide preset values for these section's settings if they feel we should.

For example: looking at the Default preset mockups, it looks like we should be using the background-2(light grey background) color system for the announcement bar instead of the setting's default which is accent-1 (black background).

@Guillaumegranger1
Copy link

The default preset/demo actually has the Background 1 color scheme for both the announcement bar and the footer:

Background = Background 1 (white)
Text = Text (dark grey)

@martinamarien
Copy link
Contributor

The default preset/demo actually has the Background 1 color scheme for both the announcement bar and the footer:

Thanks Guillaume. Is that the same for the Craft preset? Based on the mockups, the announcement bar and footer look like they use accent 1.

@Guillaumegranger1
Copy link

@martinamarien The 2nd preset uses the Accent 1 color scheme for the announcement bar and the footer. Is this problematic?

@martinamarien
Copy link
Contributor

No problem at all. We just need to know this information so that we can set the footer and announcement bar color scheme as part of the preset.

For example, right now, the footer's color scheme defaults to background-1. If we don't overwrite this setting color as part of the Craft preset, merchants will get background-1 as their footer color instead of accent-1.

Comment on lines 16 to 21
"announcement-bar": {
"type": "announcement-bar",
"settings": {
"color_scheme": "accent-1"
}
},
Copy link
Contributor

Choose a reason for hiding this comment

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

accent-1 is actually already the default value for the announcement bar color scheme. So we don't need to include this here because it will default to that color anyways.

Instead, we will need to include this for the Default preset (unless the default value of the color scheme changes for the announcement bar).

The code you have used here would work for a section that contains only one announcement bar - as in the section itself is an individual announcement bar. In this case, our announcement bar section contains blocks of type: announcement that each have their own settings. So we would need to provide overrides for each block instead of for the section as a whole. So it might looks something like this:

    "Default": {
      "sections": {
        "announcement-bar": {
          "type": "announcement-bar",
          "blocks": {
            "announcement-bar-0": {
              "type": "announcement",
              "settings": {
                  ... 
              }
            }
          }, 
          "block_order": [
            "announcement-bar-0" 
          ]
        }
      }
    }, 

Copy link
Contributor Author

Choose a reason for hiding this comment

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

accent-1 is actually already the default value for the announcement bar color scheme. So we don't need to include this here because it will default to that color anyways.

I had changed the default value in the section itself since it for the default style but I do it the way you mentioned instead.

martinamarien
martinamarien previously approved these changes Aug 17, 2021
Copy link
Contributor

@martinamarien martinamarien 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 to me.

@LucasLacerdaUX LucasLacerdaUX self-assigned this Aug 17, 2021
LucasLacerdaUX
LucasLacerdaUX previously approved these changes Aug 18, 2021
Copy link
Contributor

@LucasLacerdaUX LucasLacerdaUX left a comment

Choose a reason for hiding this comment

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

I have a minor comment, but I'm approving this as I'm not 100% sure on this one (never implemented presets myself before). Could be a bug on my store.

Please try reproducing the issue on your own store.

Comment on lines 4 to 5
"Default": {
"sections": {
Copy link
Contributor

Choose a reason for hiding this comment

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

I think you need to explicitly repeat the preset values here, otherwise you can't go back to the default preset once you change it.

Suggested change
"Default": {
"sections": {
"Default": {
"colors_solid_button_labels": "#FFFFFF",
"colors_accent_1": "#121212",
"colors_accent_2": "#334FB4",
"colors_text": "#121212",
"colors_outline_button_labels": "#121212",
"colors_background_1": "#FFFFFF",
"colors_background_2": "#F3F3F3",
"type_header_font": "lato_n3",
"type_body_font": "lato_n3",
"sections": {

To reproduce the issue:

  1. Go to Theme Settings -> Theme Style
  2. Change from Default to Craft.
  3. Save and reload the page. You can't go back to Default preset.
  4. Add the default values like I provided above. Repeat steps 1-3. It should now be possible to go back to default preset.

Maybe I'm doing something wrong or it's a bug with my store, though, so please validate before accepting this suggestion.

Copy link
Contributor

Choose a reason for hiding this comment

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

Ludo and I have both been able to reproduce this and have shared it with the appropriate team.
That said, if you change the current theme and upload it, it does show the preset correctly with the correct overrides. So I think we can be confident that the issue is not with the presets in the settings_data file.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ah but I see what Lucas means. If you set the default to Craft in settings_data.json which I think would replicate what happens when a merchant uploads a specific style from the theme store or free theme banner in the admin. Then change the style to default... it has nothing to go back to.
Video explanation (ugh my mic didn't work but basically I set the default as craft and switch back to default and nothing happens)

@ludoboludo
Copy link
Contributor Author

@LucasLacerdaUX and @martinamarien just need a re review, I added the colors and font in the preset for the Default style.

LucasLacerdaUX
LucasLacerdaUX previously approved these changes Aug 18, 2021
config/settings_data.json Outdated Show resolved Hide resolved
LucasLacerdaUX
LucasLacerdaUX previously approved these changes Aug 18, 2021
Copy link
Contributor

@LucasLacerdaUX LucasLacerdaUX left a comment

Choose a reason for hiding this comment

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

third time's the charm, I guess

Copy link

@wiktoriaswiecicka-zz wiktoriaswiecicka-zz left a comment

Choose a reason for hiding this comment

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

Looks great! ✨

@ludoboludo ludoboludo merged commit 6fa46b7 into main Aug 18, 2021
@ludoboludo ludoboludo deleted the update-presets branch August 18, 2021 14:54
tairli added a commit to tairli/dawn that referenced this pull request Aug 22, 2021
commit 399626d
Author: tairli <tairli@yahoo.com>
Date:   Mon Aug 23 04:47:25 2021 +0930

    Update product-form.js

commit 330e6b4
Merge: 79467e4 7a2f34f
Author: tairli <tairli@yahoo.com>
Date:   Mon Aug 23 04:45:54 2021 +0930

    Merge remote-tracking branch 'upstream/main' into main

commit 7a2f34f
Author: Lucas Lacerda <37168033+LucasLacerdaUX@users.noreply.github.com>
Date:   Thu Aug 19 15:55:39 2021 -0400

    Add featured product section (Shopify#261)

commit 9789ae1
Author: translation-platform[bot] <34770790+translation-platform[bot]@users.noreply.github.com>
Date:   Thu Aug 19 15:52:08 2021 -0400

    Update 2 translation files (Shopify#449)

    Co-authored-by: translation-platform[bot] <34770790+translation-platform[bot]@users.noreply.github.com>

commit d83f16f
Author: translation-platform[bot] <34770790+translation-platform[bot]@users.noreply.github.com>
Date:   Thu Aug 19 15:51:51 2021 -0400

    Update 7 translation files (Shopify#448)

    Co-authored-by: translation-platform[bot] <34770790+translation-platform[bot]@users.noreply.github.com>

commit ce0e5ec
Author: Ludo <ludo.segura@shopify.com>
Date:   Thu Aug 19 14:46:05 2021 -0400

    Hide variant images setting (Shopify#158)

commit d3b44e7
Author: Ludo <ludo.segura@shopify.com>
Date:   Thu Aug 19 10:29:50 2021 -0400

    Update font style for Default preset (Shopify#435)

    * Update font style for Default preset

    * updated default in settings_schema

commit 2643bab
Author: translation-platform[bot] <34770790+translation-platform[bot]@users.noreply.github.com>
Date:   Thu Aug 19 10:29:34 2021 -0400

    Update 15 translation files (Shopify#439)

    Co-authored-by: translation-platform[bot] <34770790+translation-platform[bot]@users.noreply.github.com>

commit e9a2fd4
Author: translation-platform[bot] <34770790+translation-platform[bot]@users.noreply.github.com>
Date:   Thu Aug 19 10:02:16 2021 -0400

    Update 4 translation files (Shopify#440)

    Co-authored-by: translation-platform[bot] <34770790+translation-platform[bot]@users.noreply.github.com>

commit 9ea0392
Author: Ludo <ludo.segura@shopify.com>
Date:   Wed Aug 18 17:19:16 2021 -0400

    404 continue shopping style (Shopify#419)

commit 6c2128a
Author: Ken Meleta <30790058+kmeleta@users.noreply.github.com>
Date:   Wed Aug 18 15:14:33 2021 -0600

    Update price sale badge spacing (Shopify#430)

commit a6d25cf
Author: Kai <KaichenWang@users.noreply.github.com>
Date:   Wed Aug 18 11:13:22 2021 -0400

    Adjust global focus styles for inputs, selects and collection filter UI elements (Shopify#389)

    * Adjust global focus styles for inputs, selects and collection filter UI elements

    * Fix focus for filter drawer summary elements

    * Simplify focus for elements that are always "focus-visible" when in focus

    * Adjust focus style of search submit button

commit 002b6b0
Author: Ken Meleta <30790058+kmeleta@users.noreply.github.com>
Date:   Wed Aug 18 09:01:47 2021 -0600

    Product card spacing and price polish (Shopify#407)

    * Adjust product card spacing and price styles

    * Fix unit price/sale badge spacing

    * Revert flex gap approach

commit 6fa46b7
Author: Ludo <ludo.segura@shopify.com>
Date:   Wed Aug 18 10:54:58 2021 -0400

    Update Preset default (Shopify#410)

commit 341e051
Author: translation-platform[bot] <34770790+translation-platform[bot]@users.noreply.github.com>
Date:   Wed Aug 18 10:43:37 2021 -0400

    Update 10 translation files (Shopify#427)

    Co-authored-by: translation-platform[bot] <34770790+translation-platform[bot]@users.noreply.github.com>

commit 79467e4
Merge: 42cad96 789c3d9
Author: tairli <tairli@yahoo.com>
Date:   Wed Aug 18 19:26:00 2021 +0930

    Merge branch 'Shopify:main' into main

commit 42cad96
Merge: b5ddb38 2ba2725
Author: tairli <tairli@yahoo.com>
Date:   Tue Aug 17 22:06:29 2021 +0930

    Merge branch 'Shopify:main' into main
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
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Update Preset #1 + add Preset #2
5 participants