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(search): ES-1192 fixed the replace helper to expand and collapse the product filters #1689

Merged
merged 1 commit into from
Jun 15, 2020

Conversation

bc-krishsenthilraj
Copy link
Contributor

@bc-krishsenthilraj bc-krishsenthilraj commented Jun 12, 2020

What?

Product filters are not expanded or collapsed in category listing, brand listing & search results page when product filters for SF are turned on.

Why

Currently, we are not able to expand/collapse the filters since, replace helper returns empty string when the input does not have the special character & in it. Thus collapsible divs are having same id and thus its broken this feature in the product filters.

In the below image, both the data-collapsible in the html source is same for both Catrgory & Brand filters.

image

Tickets / Documentation

Screenshots (if appropriate)

Steps:

  1. Login to CP
  2. Use the theme file (Cornerstone-4.6.1_fix_ES-1192.zip) uploaded in the above issue
  3. Upload & Publish the theme by navigating to Storefront
  4. Navigate to Products, choose any product
  5. Create a custom field with special character & in it. Ex: Custom & Filter and save the product.
  6. Navigate to Products -> Product Filters and enable product filters
  7. Make sure to enable above created custom filter and save.
  8. Go to SF and check by expand & collapsing all the product filters.
  9. All the filters should be collapsible or expandable.

Images:

  1. Before collapasing any filter

image

  1. Category filter collapsed

image

  1. Category & Brand filters collapsed

image

  1. Category Expanded, but Brand collapsed

image

  1. Other & Custom filter expanded
    image

  2. Other collapsed and Custom expanded
    image

  3. Both Other & Custom filters collapsed
    image

  4. Other Expanded again & Custom filter collapsed
    image

  5. Both Other & Custom expanded again
    image

  6. All the filters collapsed
    image

@bigbot
Copy link

bigbot commented Jun 12, 2020

Autotagging @bigcommerce/storefront-team @davidchin

lord2800
lord2800 previously approved these changes Jun 12, 2020
Copy link
Contributor

@lord2800 lord2800 left a comment

Choose a reason for hiding this comment

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

@@ -3,7 +3,7 @@
<div
class="accordion-navigation toggleLink {{#unless ../start_collapsed }} is-open {{/unless}}"
role="button"
data-collapsible="#facetedSearch-content--{{#replace '&' (hyphenate facet) }}{{/replace}}">
data-collapsible="#facetedSearch-content--{{#replace '&' (concat '&' (hyphenate facet)) }}{{/replace}}">
Copy link
Contributor

@junedkazi junedkazi Jun 12, 2020

Choose a reason for hiding this comment

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

Sorry I am not sure I understand what is going on here and what are we trying to do. Can you please an explanation to the description as to why were they broken in the first place. Also I am not sure I understand why we need to add concat '&'. Can you please explain using this example Custom & Filter. If I am reading correctly it will look something like this;
Custom & Filter -> hyphenate Custom-&-Filter -> concat &Custom-&-Filter -> replace Custom-Filter

Please correct me if I am wrong.

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 replace helper, returns empty string if it doesn't find any & in it. Thus all the standard filters are returned as empty string. So, we fixed it by concatenating & to all the input facets and replace them later.

Since custom filters are part of this loop, we ended up adding and removing the & to it.

Copy link
Contributor

Choose a reason for hiding this comment

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

Open to suggestions here but what happens is if replace doesn't find the replacement character, it returns an empty string. So what happens is Custom Filter becomes: 'Custom Filter' -> hyphenate 'Custom-Filter' -> replace ''. The concat forces there to be at least one replacement token, so it always exists.

Copy link
Contributor

Choose a reason for hiding this comment

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

It feels so hacky and for input's like this Custom & Filter it will generate output with double dashCustom--Filter.

Copy link
Contributor

Choose a reason for hiding this comment

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

I agree, but the whole way the expand/collapse system works using css selectors also feels just as hacky to me. I'm open to suggestions here, once again.

Copy link
Contributor

Choose a reason for hiding this comment

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

I think it should no longer return empty string based on https://github.com/bigcommerce/paper-handlebars/pull/92/files

Copy link
Contributor

Choose a reason for hiding this comment

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

OK, I figured out what the disconnect was here. We needed to use {{replace}} with {{else}}, which is beyond confusing. 😕

@bc-aodvak
Copy link

Executed steps above for search, filter by category, filter by brand. Looks good 💚

@bc-williamkwon bc-williamkwon merged commit ae514d8 into bigcommerce:master Jun 15, 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.

None yet

6 participants