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-4679 - fixing product layout when sidebar off #1205

Merged
merged 1 commit into from
Apr 23, 2018

Conversation

tpietsch
Copy link
Contributor

@tpietsch tpietsch commented Apr 20, 2018

What?

Added the theme setting check for the shop by price sidebar

Tickets / Documentation

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

Screenshots (if appropriate)

screen shot 2018-04-20 at 9 34 48 am
screen shot 2018-04-20 at 9 34 55 am

@bigcommerce/platform-engineering @bigcommerce/storefront-team

@bigbot
Copy link

bigbot commented Apr 20, 2018

Autotagging @bigcommerce/storefront-team @davidchin

{{> components/category/sidebar}}
</aside>
{{/or}}
{{#if theme_settings.shop_by_price_visibility}}
Copy link
Contributor

Choose a reason for hiding this comment

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

This won't work. Currently it is just shop by price but if you have faceted search enabled there are other elements in the side bar https://github.com/bigcommerce/cornerstone/blob/67305e81473f01a93a351ad4b2e80b60fc9c66ba/templates/components/category/sidebar.html.

This change will hide those blocks as well.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

figured as such - fixed

@@ -18,3 +20,4 @@ <h5 class="sidebarBlock-heading">{{category.name}}</h5>
{{> components/category/shop-by-price shop_by_price=category.shop_by_price category_url=category.url}}
{{/if}}
</nav>
</aside>
Copy link
Contributor Author

Choose a reason for hiding this comment

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

verified sidebar is only used by category.html

{{#if theme_settings.shop_by_price_visibility}}
{{> components/category/sidebar}}
{{/if}}
{{/if}}
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@junedkazi fixed to handle the cases separately. if you have suggestions or if you can point me in the direction of whatever templating lang this is so i can read up.

Copy link
Contributor

Choose a reason for hiding this comment

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

{{> components/category/shop-by-price shop_by_price=category.shop_by_price category_url=category.url}}
{{/if}}
</nav>
</aside>
Copy link
Contributor Author

Choose a reason for hiding this comment

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

verified this is only used by the file im changing - also not sure why the diff is showing like this all i did was move the aside tag in here

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

@mattolson mattolson Apr 20, 2018

Choose a reason for hiding this comment

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

I think the if/elseif below can be moved into this #or block helper. We import most of https://github.com/helpers/handlebars-helpers#helpers so you can use #or and #and together to achieve the same thing.

Try this:
#or category.subcategories category.faceted_search_enabled (and category.shop_by_price theme_settings.shop_by_price_visibility)

Copy link
Contributor

Choose a reason for hiding this comment

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

Try this instead:

{{#any category.subcategories category.faceted_search_enabled (all category.shop_by_price theme_settings.shop_by_price_visibility)}}

Copy link
Contributor Author

Choose a reason for hiding this comment

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

both have the "TypeError: Uncaught error: opts.inverse is not a function" error

Copy link
Contributor

Choose a reason for hiding this comment

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

Ok, forget that, I guess you need an else clause for that to work. I would just go back to your current solution, but can you get rid of the redundant #or that wraps all this?

@mattolson mattolson dismissed junedkazi’s stale review April 23, 2018 17:18

Has been addressed

@mattolson
Copy link
Contributor

@tpietsch Looks good. Can you add an entry to the changelog?

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