Skip to content
This repository has been archived by the owner on Jun 1, 2023. It is now read-only.

Fix --live parameter, it should not imply --allow-live in the theme push command #1807

Merged
merged 2 commits into from
Dec 1, 2021

Conversation

karreiro
Copy link
Contributor

@karreiro karreiro commented Nov 30, 2021

WHY are these changes introduced?

We reconsidered this change considering @ADTC feedback.

WHAT is this pull request doing?

Now, the --live parameter does not imply --allow-live in the theme push command anymore.

How to test your changes?

Run shopify theme push --live and see the confirmation message:

image

Update checklist

  • I've added a CHANGELOG entry for this PR (if the change is public-facing)
  • I've considered possible cross-platform impacts (Mac, Linux, Windows).
  • I've left the version number as is (we'll handle incrementing this when releasing).
  • I've included any post-release steps in the section above.

@karreiro karreiro requested review from macournoyer, a team, pepicrft and amcaplan and removed request for a team November 30, 2021 17:04
@ADTC
Copy link
Contributor

ADTC commented Nov 30, 2021

Is there any way to emit the theme name and ID before the question? This way, we can be sure which theme is being pushed to. Otherwise we're flying blind.

@karreiro
Copy link
Contributor Author

@ADTC I'm not sure if I've understood the suggestion correctly. But, the theme being pushed is always your local theme.

It's a bit different from when, for example, we execute shopify theme publish to publish one of the remote themes.

With shopify theme publish is important to show the name and the ID because we're selecting on of the remote themes. While with shopify theme push, we're always handling the local theme.

Am I missing something?

@ADTC
Copy link
Contributor

ADTC commented Nov 30, 2021

But, the theme being pushed is always your local theme.

Actually we're pushing the local theme files into a remote theme. If you execute shopify theme push it asks you to select one of the remote themes too. I use it everyday to push to remote themes, both live and unpublished.

I'm not sure what publish does. I think it creates a new remote theme? Can't check right now as I'm mobile. Ah. I think publish just publishes one of the unpublished themes, making it live. Right? (It doesn't push anything from local.)

Anyway, when the prompt asks "Are you sure you want to push to the live theme?" I want to know, which theme is Shopify CLI seeing as the live theme right now? Hence the suggestion to show the theme name and ID before the question.

@ADTC
Copy link
Contributor

ADTC commented Nov 30, 2021

If I didn't use the --live option and I was presented with a list of themes, and I chose the live theme, I think there will be a line that says "You chose Theme Name (#ThemeID)" before the question "Are you sure you want to push to your live theme?". But with --live, the live theme is silently selected. It will be better if the CLI showed a line "Live theme is Theme Name (#ThemeID)" in this case as well

@karreiro
Copy link
Contributor Author

Ohh! Thanks for the clarification, @ADTC.

We actually want to show the name and ID of the theme we're about to replace (the current live theme), correct?

I created this issue to handle this #1809. Could you please take a look to ensure the expected message is good enough for what we want to accomplish? 🚀

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Feature request: theme pull and theme push to have a --live (-l) option to select the current live theme
3 participants