-
Notifications
You must be signed in to change notification settings - Fork 613
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
changed access level for parent context- products list display #1384
Conversation
Autotagging @bigcommerce/storefront-team @davidchin |
@@ -2,9 +2,9 @@ | |||
{{#each products}} | |||
<li class="product"> | |||
{{#if settings.data_tag_enabled}} | |||
{{>components/products/list-item show_compare=../show_compare show_rating=../settings.show_product_rating theme_settings=../theme_settings customer=../customer event=../event position=(add @index 1)}} | |||
{{>components/products/list-item show_compare=../../show_compare show_rating=../../settings.show_product_rating theme_settings=../../theme_settings customer=../../customer event=../../event position=(add @index 1)}} |
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.
Can you elaborate on this change? Why do we need to do it here and not in any of the other templates?
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.
@mattolson Entering into an if statement adds another level to the context, so the parent becomes ../../foo
Looking on the grid display, I do see that the other values also will be available in the same context as show_compare so they will also should to be modified. I also tested @index and this variable is available and no change needed for it.
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.
If that is the case, then many other things would be broken too. This if statement was added in #1377 which added it in many other places as well. Are those templates experiencing issues too?
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.
@mattolson I looked on pr 1377. The cases that I can see that we used reference to parent context inside if statements are in the following:
https://github.com/bigcommerce/cornerstone/pull/1377/files#diff-a6bce6406ef20c5a779702dccc5173b7R2
https://github.com/bigcommerce/cornerstone/pull/1377/files#diff-53b336f69d0af6f9498a97945e1a5f60R248
https://github.com/bigcommerce/cornerstone/pull/1377/files#diff-f8e0227af0d7c280e2ac9ffdc6f746d8R27
https://github.com/bigcommerce/cornerstone/pull/1377/files#diff-135cdd50e87e5a25fb7b2a86965ae978R45
I will take a look into the each case to see if we did refer to the parent context correctly.
@lord2800 can you take a look at this since it is a modification of some work you did earlier? |
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.
Sorry this took so long to get to. @bc-zoharmuzafi and I sat down and figured out that there was more than one bug here, and we were able to eliminate a redundant if
check in the process.
What?
In Cornerstone v2.6.0 compare buttons are missing from Category pages that use a List view instead of a grid. This pr change the access level to the parent context when generating the products list display.
Screenshots (if appropriate)
Before
After