-
Notifications
You must be signed in to change notification settings - Fork 221
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
graygilmore
left a comment
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.
d6d3181 to
202158a
Compare
karreiro
left a comment
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.
Thank you for this PR, @chrisberthe! Excellent work! 🚀
I’ve tested it, and most workflows are working quite reliably. However, one workflow isn’t behaving as expected:
shopify theme dev --listing example --theme-editor-sync
When we run the command with the --theme-editor-sync flag, we expect that changes made in the theme editor are automatically downloaded. Currently, with the new flag, those changes seem to be lost.
If supporting these flags together turns out to be too challenging, we could consider throwing an early error when both are used at the same time. Ideally, though, it would be great to make them work together if possible. Please let me know your thoughts.
Thanks again for all your work on this PR!
|
Thanks @karreiro. I'm taking vacation so I'll look into this once I'm back in a week or so. Could you share some thoughts on this point? |
|
This PR seems inactive. If it's still relevant, please add a comment saying so. Otherwise, take no action. |
|
Commenting so that this won't get closed automatically. |
|
This PR seems inactive. If it's still relevant, please add a comment saying so. Otherwise, take no action. |
|
Any progress on this? |
5a88ae9 to
db3c204
Compare
This fell by the wayside. Will look into the remaining feedback and address. |
|
@karreiro Going to need your input on
Personally, leaning towards the latter. What do you think? |
db3c204 to
33c5271
Compare
f19b1db to
f64660c
Compare
|
This PR seems inactive. If it's still relevant, please add a comment saying so. Otherwise, take no action. |
|
Still relevant. |
|
Can't say when this will be merged @anastis, but I can say that having to repeatedly rebase this is getting annoying 😛 |
|
This PR seems inactive. If it's still relevant, please add a comment saying so. Otherwise, take no action. |
|
Still relevant. |
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
/listingsdirectory 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?
--listingflag toshopify theme dev,shopify theme push, andshopify theme sharecommandsconfig/settings_data.jsonpreset 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