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

STRF-3860: Fixed Shop by Price visibility when toggle is disabled in … #1161

Merged
merged 2 commits into from
Feb 12, 2018

Conversation

adamdyche
Copy link
Contributor

@adamdyche adamdyche commented Feb 1, 2018

What?

Fixes the Show "Shop by Price" toggle within the Theme Editor for Cornerstone so that the Shop by Price section on the Category pages will not display when the toggle is disabled. When the Show "Shop by Price" toggle is disabled in the Theme Editor it will still accurately display the Price section when enabled via Faceted Search.

Tickets / Documentation

https://jira.bigcommerce.com/browse/STRF-3860

Screenshots (if appropriate)

Shop by Price Enabled

shop by price enabled

Shop by Price Disabled

shop by price disabled

Shop by Price Disabled with Faceted Search Enabled

shop by price disabled with faceted search enabled

@junedkazi @PascalZajac

@bigbot
Copy link

bigbot commented Feb 1, 2018

Autotagging @bigcommerce/storefront-team @davidchin

Copy link
Contributor

@PascalZajac PascalZajac left a comment

Choose a reason for hiding this comment

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

Some minor comments/questions.

CHANGELOG.md Outdated
@@ -4,6 +4,7 @@
- Fix logo not loading on order confirmation page [#1159](https://github.com/bigcommerce/cornerstone/pull/1159)
- Add support in Cornerstone to consume AMP ID [#1155](https://github.com/bigcommerce/cornerstone/pull/1155)
- Fix option selection reset bug when a variation is out of stock [#1160](https://github.com/bigcommerce/cornerstone/pull/1160)
- Fix "Shop by Price" toggle in theme editor to hide Shop by Price when faceted search is not enabled.
Copy link
Contributor

Choose a reason for hiding this comment

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

You can add the PR link here now that it exists.

@@ -55,7 +55,7 @@
"productpage_related_products_count": 10,
"productpage_similar_by_views_count": 10,
"categorypage_products_per_page": 12,
"shop_by_price_visible": true,
Copy link
Contributor

Choose a reason for hiding this comment

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

I believe the existing configuration values for this field are saved in the TR DB, so renaming the field effectively wipes out all existing merchant settings, is that right?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thank you for pointing that out. I found a better way to implement the change without changing the TR DB value.

@@ -24,16 +24,10 @@ <h1 class="page-heading">{{category.name}}</h1>
{{{category.description}}}
{{{snippet 'categories'}}}
<div class="page">
{{#or category.subcategories category.faceted_search_enabled}}
{{#or category.subcategories category.faceted_search_enabled category.shop_by_price}}
Copy link
Contributor

Choose a reason for hiding this comment

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

This should probably be a simple if check now right? Since it's not like an or / else.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This has always been an or statement as I ended up reverting the changes from 1036. I did try to change it to an if upon doing so though it did generate a 500 internal server error with stapler.

Copy link
Contributor

Choose a reason for hiding this comment

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

Ah yeah that's my bad, I didn't understand what the or helper was actually doing.

@PascalZajac
Copy link
Contributor

👍this looks fine to me but I'll leave final approval to the STRF guys.

CHANGELOG.md Outdated
@@ -4,6 +4,7 @@
- Fix logo not loading on order confirmation page [#1159](https://github.com/bigcommerce/cornerstone/pull/1159)
- Add support in Cornerstone to consume AMP ID [#1155](https://github.com/bigcommerce/cornerstone/pull/1155)
- Fix option selection reset bug when a variation is out of stock [#1160](https://github.com/bigcommerce/cornerstone/pull/1160)
- Fix "Shop by Price" toggle in theme editor to hide Shop by Price when faceted search is not enabled. [#1161](https://github.com/bigcommerce/cornerstone/pull/1161)
Copy link
Contributor

Choose a reason for hiding this comment

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

This entry should go in the draft section.

Copy link
Contributor

Choose a reason for hiding this comment

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

Please rebase your branch.

Copy link
Contributor Author

@adamdyche adamdyche Feb 6, 2018

Choose a reason for hiding this comment

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

I was able to rebase and move the entry into the draft section @junedkazi .

@adamdyche adamdyche force-pushed the STRF-3860 branch 2 times, most recently from 983dda8 to 2cbd25f Compare February 6, 2018 02:42
@junedkazi junedkazi merged commit 67305e8 into bigcommerce:master Feb 12, 2018
@adamdyche adamdyche deleted the STRF-3860 branch February 12, 2018 22:32
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.

None yet

4 participants