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-8630 update cornerstone to be compatible with handlebars v4 #1834

Merged
merged 1 commit into from
Oct 12, 2020

Conversation

jairo-bc
Copy link
Contributor

@jairo-bc jairo-bc commented Sep 10, 2020

What?

Updated cornerstone theme to be compatible with the handlebars v4. Most changes are applied due to changed logic in the calculating ../ and = character is now HTML escaped.
https://github.com/handlebars-lang/handlebars.js/blob/master/release-notes.md#v400---september-1st-2015

Tickets / Documentation

https://jira.bigcommerce.com/browse/STRF-8626
https://jira.bigcommerce.com/browse/STRF-8630

How it was tested?

Several times tried to do that using functional tests. Couldn't get a case, when the tests weren't failing. I've got, that latest master(e1270850b1b083d762b339e57096b2c06be53ef3) and these changes have same amount errors(except 1 case on master). Links I'm able to provide in private

@bigbot
Copy link

bigbot commented Sep 10, 2020

Autotagging @bigcommerce/storefront-team @davidchin

@jairo-bc jairo-bc requested review from a team September 10, 2020 14:49
default_image=../../theme_settings.default_image_product
fallback_size=../theme_settings.productthumb_size
lazyload=../theme_settings.lazyload_mode
default_image=../theme_settings.default_image_product
Copy link
Contributor

Choose a reason for hiding this comment

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

Does @root work in v4? It might make for cleaner syntax if we just did @root.theme_settings.default_image_product for example.

https://handlebarsjs.com/api-reference/data-variables.html

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Didn't check, will take a look.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Just checked this, for some reason @root is undefined for v3 and v4.

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.

How have you tested this?

@jairo-bc
Copy link
Contributor Author

How have you tested this?

Mostly manually. Took a look at all occurrences of the ../ in the codebase and checked if the are inside #if or other block helpers, that have changed the context behaviour.

I think after stencil-cli is released with newer version I will be able to run functional tests on integration. Does it take specific version or the latest master? @junedkazi

@jairo-bc
Copy link
Contributor Author

@junedkazi @mattolson Added to the description findings from the testings

@junedkazi
Copy link
Contributor

@jairo-bc this need a rebase

@junedkazi junedkazi merged commit 0564704 into bigcommerce:master Oct 12, 2020
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.

6 participants