-
Notifications
You must be signed in to change notification settings - Fork 184
Add "listing" flag support to theme commands #6007
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
base: main
Are you sure you want to change the base?
Add "listing" flag support to theme commands #6007
Conversation
5a32b1e
to
881d68a
Compare
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.
Testing this out with theme dev
worked well! Very neat! Couple of pieces of feedback and I'll ping the rest of the team to get one more set of eyes because I'm not too familiar with some of the pieces in here.
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'm not seeing any validation around if a user provides a --listing
value that doesn't exist. I think we should stop the command and return an error otherwise they'd be left assuming their command worked but we actually never found a matching listing.
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.
Fair point. I originally had a number of validation checks for this and ultimately decided on removing as I wasn't sure where to draw the line with what the CLI validates for versus the platform.
Currently, if you specify a non-existent listing, the platform will output an error about this, i.e. settings_data.json error when the "current" key is set to a non-existent preset/listing, but this will only happen after the dev server boots up, not before, which perhaps is less ideal than throwing an error before files are synced.
881d68a
to
d6d3181
Compare
d6d3181
to
202158a
Compare
Follow up to #6002.
WHY are these changes introduced?
Shopify introduced new requirements for the Shopify Theme Store that require themes with multiple presets to include a
/listings
directory in their theme zip submissions. However, developers need a way to preview and test these specific theme presets during development and deployment.WHAT is this pull request doing?
--listing
flag toshopify theme dev
,shopify theme push
, andshopify theme share
commandsconfig/settings_data.json
preset switching, i.e."current" key
, based on listing nameHow to test your changes?
Create a theme with a
listings/
directory:Test development with listing:
Test push with listing:
Test share with listing:
Verify that listing-specific files are served when they exist, and base theme files are used as fallbacks when they don't
Measuring impact
How do we know this change was effective? Please choose one:
Checklist