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

feat(storefront): STRF-9258 Stencil Pull: Provide an activate option #739

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

jairo-bc
Copy link
Contributor

What?

Refactored Stencil Pull command to have ability write unit tests.
Added activate option for Stencil Pull

Tickets / Documentation

@jairo-bc jairo-bc force-pushed the STRF-9258 branch 3 times, most recently from f592db5 to 66ce9e1 Compare July 27, 2021 08:06
@jairo-bc jairo-bc changed the title feat: strf-9258 Stencil Pull: Provide an activate option feat(storefront): STRF-9258 Stencil Pull: Provide an activate option Jul 28, 2021
Copy link
Contributor

@mattolson mattolson left a comment

Choose a reason for hiding this comment

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

Code looks good, but not sure about the naming of the option. If I understand correctly, the new option overrides the variation to use when fetching the configuration to merge. So it doesn't really "activate" a variation, it merges the given variation's configuration (as opposed to the default variation).

Thinking about this functionality more, I'm not sure it's working in the best possible way. When we merge these configurations, we put it into the top-level "settings" object. However, the variations still exist in the config file, and when this is re-uploaded, those variations will be layered on top of the base settings object. So if you think about the full experience of stencil pull + edits + stencil push against the Cornerstone Bold variations for example, we would pull the existing Bold configuration, merge it into the default settings object, but because the Bold variation still exists in config.json, when re-uploaded, we wouldn't see those settings, we would see whatever is still leftover in the Bold variation (a separate object in config.json).

@jairo-bc
Copy link
Contributor Author

jairo-bc commented Aug 3, 2021

@mattolson Maybe the name is not the best. I'm using the same as we currently have for stencil push (https://github.com/bigcommerce/stencil-cli/blob/master/bin/stencil-push.js)

Regarding functional approach.
Currently if you run stencil push, it will update only global settings. If you run stencil push --activate Bold, it will update only Bold variation.
If you run stencil pull, it will update only global settings.

And I'm adding activate option for stencil pull to have consistency with stencil push

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