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

feature: Layered Navigation (Filters) #797

Merged
merged 134 commits into from
Jun 3, 2019

Conversation

codeAdrian
Copy link
Contributor

@codeAdrian codeAdrian commented Jan 31, 2019

Description

When this PR is merged, it will:

  • Add a Layered navigation (Filters) modal where users can choose, apply and reset filters.
  • Filters data is pulled from Products data.
  • GraphQL currently doesn't support filters being applied to queries.
  • URL is being updated with applied filters
  • Applied filters are initialized with URL data
  • Config for each filter group (grid/list mode, search, etc.) is partially hard-coded in constants.js file
  • Users are able to search the groups of filters (Discussed with @soumya-ashok in meeting). Currently this is applied only to "Fashion color" filters, but it can be enabled for any group.

Additional notes:

  • Tests will be added when general components and structure is approved
  • There are some issues with unmounting filter swatch components on change. I haven't been able to trace the issue and fix it. Need asssistance

Related Issue

Closes #361

Motivation and Context

Adding Layered navigation to category page.

How Has This Been Tested?

Tested with Venia sample data on any product category.

Automatic tests will be added when general components and structure are approved.

Screenshots (if appropriate):

screen shot 2019-01-31 at 1 42 02 pm

Proposed Labels for Change Type/Package

FEATURE

venia-concept

Checklist:

  • I have read the CONTRIBUTING document.
  • I have linked an issue to this PR.
  • I have indicated the change type and relevant package(s).
  • I have updated the documentation accordingly.
  • I have added tests to cover my changes.
  • All new and existing tests passed.
  • All CI checks are green (linting, build/deploy, etc).
  • At least one core contributor has approved this PR.

@codeAdrian
Copy link
Contributor Author

Hello @soumya-ashok thank you for feedback.

Regarding the filters not applying after changing, I'm not sure if the attribute filters input has been added to GraphQL backend. @jimbo , @sirugh feel free to let me know if I'm in the wrong. I've been checking out the Catalog GraphQL schema and didn't see the attribute filter input: https://github.com/magento/magento2/blob/2.3-develop/app/code/Magento/CatalogGraphQl/etc/schema.graphqls#L279

Regarding the style issues (full height filters modal and swatch width), they have been fixed in the latest update. Let me know if there is anything else that needs to be fixed.

@dpatil-magento
Copy link
Contributor

dpatil-magento commented May 17, 2019

Thanks @codeAdrian fixes looks good. Waiting for @jimbo to provide review approval for recent commits.

@sirugh sirugh removed their assignment May 21, 2019
@awilcoxa awilcoxa removed this from the Next PWA Release (Core 2.3.2) milestone May 22, 2019
@cherdman cherdman assigned zetlen and unassigned jimbo May 22, 2019
@sirugh
Copy link
Contributor

sirugh commented May 27, 2019

Fails
🚫

node` failed.

Log

Error:  Error: Could not find a `dangerfile.js` or `dangerfile.ts` in the current working directory.
    at Object.dangerfilePath (/codebuild/output/src451119981/src/github.com/magento-research/pwa-studio/node_modules/danger/distribution/commands/utils/fileUtils.js:26:11)
    at Object.<anonymous> (/codebuild/output/src451119981/src/github.com/magento-research/pwa-studio/node_modules/danger/distribution/commands/danger-runner.js:92:42)
    at step (/codebuild/output/src451119981/src/github.com/magento-research/pwa-studio/node_modules/danger/distribution/commands/danger-runner.js:33:23)
    at Object.next (/codebuild/output/src451119981/src/github.com/magento-research/pwa-studio/node_modules/danger/distribution/commands/danger-runner.js:14:53)
    at fulfilled (/codebuild/output/src451119981/src/github.com/magento-research/pwa-studio/node_modules/danger/distribution/commands/danger-runner.js:5:58)
    at process._tickCallback (internal/process/next_tick.js:68:7)

Generated by 🚫 dangerJS against 0e122d3

@magento-cicd2
Copy link

CLA assistant check
Thank you for your submission, we really appreciate it. Like many open source projects, we ask that you all sign our Contributor License Agreement before we can accept your contribution.
1 out of 2 committers have signed the CLA.

✅ Starotitorov
❌ Adrian Bece


Adrian Bece seems not to be a GitHub user. You need a GitHub account to be able to sign the CLA. If you have already a GitHub account, please add the email address used for this commit to your account.

jimbo
jimbo previously approved these changes May 30, 2019
Copy link
Contributor

@jimbo jimbo left a comment

Choose a reason for hiding this comment

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

@codeAdrian Thanks for being so patient with us. This is in a great place now, and I'm happy to approve it.

When we're ready to hook this up to GraphQL, we'll have another opportunity to adjust the UX or presentation of the components, but I think this is ready to go. 👍

@jimbo
Copy link
Contributor

jimbo commented Jun 1, 2019

@codeAdrian I've resolved the conflicts that were created when we merged #1211, which converted Category to a functional component using hooks. Take a look and see if you have any feedback; otherwise we'll try to merge this Monday.

@codeAdrian
Copy link
Contributor Author

@jimbo Thanks for resolving the conflicts. I really appreciate it. I have fixed a minor issue and cleaned up a commented-out piece of code that was leftover from the merge. I am happy to proceed with the merge on Monday. Cheers!

@dpatil-magento
Copy link
Contributor

Did another round of QA, all looks good. @jimbo - Good to merge.

Copy link
Contributor

@jimbo jimbo left a comment

Choose a reason for hiding this comment

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

Great, this is good to go now.

I'd like to follow this up with a new PR to migrate the filter logic from Redux into a hook. @tjwiebell had successfully moved the pagination logic into a hook in #1211, which meant Category no longer needed to be connected to Redux, but with this PR it's connected again. If we put the filter logic in a hook, we can once again remove that connection to Redux.

@jimbo jimbo merged commit f1114a1 into magento:develop Jun 3, 2019
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.

Implement filtering for Category with Venia (PWA)