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

BIG-16069: Faceted search component #158

Merged
merged 11 commits into from
Jul 10, 2015
Merged

BIG-16069: Faceted search component #158

merged 11 commits into from
Jul 10, 2015

Conversation

davidchin
Copy link
Contributor

Faceted search component UI. There are several things to check:

  1. Facet accordion - respecting user preference
  2. Facet items "show more" toggle - respecting user preference
  3. Different facet types - hierarchy, rating, multi and range
  4. Mobile / desktop variation
  5. AJAX loading state
  6. State restoration after AJAX partial injection

It does not exactly match the design - for example, color options and price range slider, which could be worked on separately.

@bc-chris-roper @haubc @christopher-hegre

@bc-chris-roper
Copy link
Contributor

@bc-miko-ademagic @SiTaggart

border-bottom-width: 0;
}

&.is-open > .accordion-navigation {
Copy link
Contributor

Choose a reason for hiding this comment

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

.accordion--navList > .accordion-block:last-child.is-open > .accordion-navigation { ... }

WOW much specificity

Copy link

Choose a reason for hiding this comment

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

hmm

Copy link
Contributor Author

Choose a reason for hiding this comment

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

lol, yeah pretty crazy. I tried really hard using foundation.accordion.js, which I ended up with nasty nested accordions and didn't produce the result I wanted. Anyway, in b99da27 I decided to not use foundation.accordion.js and created a generic Collapsible class to handle collapse/expand interaction. So now all the child descendant selectors are no longer needed.

@ghost
Copy link

ghost commented Jul 8, 2015

Tested locally and this looks really good so far: https://dl.dropboxusercontent.com/spa/jd5nszcz7axrfas/96849__p.png

@ghost
Copy link

ghost commented Jul 8, 2015

Not sure if we ever received a design for the AJAX loading state indicator but perhaps it should be more visible somewhere, dimming the entire product results box or something like that.

@ghost
Copy link

ghost commented Jul 8, 2015

@christopher-hegre haven't received the loading indicator yet, but it's been requested now.

@davidchin
Copy link
Contributor Author

I revised the PR based on the feedback. Please note that it now requires the change in bigcommerce/paper#18

@SiTaggart @bc-chris-roper @haubc @christopher-hegre

//
// -----------------------------------------------------------------------------
.toggleLink {
.toggleLink-text--off {
Copy link
Contributor

Choose a reason for hiding this comment

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

Does this need to be nested?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

No they don't. I've updated it.

@SiTaggart
Copy link
Contributor

Just some of those minor things, but on the whole looks good

@davidchin
Copy link
Contributor Author

@SiTaggart so I also configured .accordion styling so that generic classes can work on their own, even without the modifier.

@SiTaggart
Copy link
Contributor

👍 Awesome. Heaps better. Only stylistic thing is a clear line break under each comment block in the scss files as per the style guide, but I'll just merge for now.

SiTaggart added a commit that referenced this pull request Jul 10, 2015
BIG-16069: Faceted search component
@SiTaggart SiTaggart merged commit def58f2 into bigcommerce:master Jul 10, 2015
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

3 participants