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.

Sign up for GitHub

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

Jump to bottom

Category RootComponent Simple Re-Factor #1211

Merged
merged 14 commits into from
May 30, 2019
Merged

Category RootComponent Simple Re-Factor #1211

merged 14 commits into from
May 30, 2019

Conversation

tjwiebell
Copy link
Contributor

Description

Re-factor Category RootComponent and all components inside component directory to be functional components that use hooks.

Related Issue

Closes #975

Verification Steps

  1. Go to the Bottoms Category
  2. Verify that a product list renders

How Have YOU Tested this?

Manually ran steps above and ran test suite

Screenshots / Screen Captures (if appropriate):

Proposed Labels for Change Type/Package

  • major (e.g x.0.0 - a breaking change)
  • minor (e.g 0.x.0 - a backwards compatible addition)
  • patch (e.g 0.0.x - a bug fix)

Checklist:

  • I have updated the documentation accordingly, if necessary.
  • I have added tests to cover my changes, if necessary.

@vercel
Copy link

vercel bot commented May 7, 2019 •
edited
Loading

This pull request is automatically deployed with Now.
To access deployments, click Details below or on the icon next to each push.

Latest deployment for this branch: https://venia-git-feature-category-peregrine.magento-research1.now.sh

sirugh
sirugh reviewed May 9, 2019
View reviewed changes
Copy link
Contributor

@sirugh sirugh left a comment

Choose a reason for hiding this comment

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

Just one comment, otherwise 👍

jimbo
jimbo suggested changes May 9, 2019
View reviewed changes
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.

@tjwiebell Nice work! This looks great so far, just a few changes to make.

packages/venia-concept/src/RootComponents/Category/category.js Outdated Show resolved Hide resolved
packages/venia-concept/src/RootComponents/Category/container.js Outdated Show resolved Hide resolved
packages/venia-concept/src/RootComponents/Category/category.js Outdated Show resolved Hide resolved
packages/venia-concept/src/RootComponents/Category/category.js Outdated Show resolved Hide resolved
@sirugh sirugh assigned tjwiebell and unassigned sirugh May 10, 2019
tjwiebell added 2 commits May 20, 2019 09:11
@tjwiebell
Merge remote-tracking branch 'mainline/develop' into feature/category…
58bf86c 
…-peregrine
@tjwiebell
- Move pagination logic to custom hook
d54790e 
- Refactor Category component to use this hook and simplify render logic
@tjwiebell tjwiebell assigned jimbo and unassigned tjwiebell May 20, 2019
jimbo
jimbo previously approved these changes May 21, 2019
View reviewed changes
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.

@tjwiebell Nicely done! This looks great to me. 🎉

Let's get it in QA and get it merged.

zetlen
zetlen reviewed May 22, 2019
View reviewed changes
Copy link
Contributor

@zetlen zetlen left a comment

Choose a reason for hiding this comment

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

This does look awesome; thank you @tjwiebell. I had one question about scroll triggers (and it's for @jimbo and/or @soumya-ashok to answer too), but that's it.

packages/peregrine/src/hooks/usePagination.js Show resolved Hide resolved
packages/venia-concept/src/RootComponents/Category/category.js Outdated Show resolved Hide resolved
@tjwiebell tjwiebell requested review from sirugh and zetlen May 22, 2019 21:35
@tjwiebell tjwiebell assigned sirugh and unassigned jimbo May 22, 2019
sirugh
sirugh previously approved these changes May 23, 2019
View reviewed changes
@sirugh sirugh removed their assignment May 23, 2019
@tjwiebell tjwiebell added the version: Major This changeset includes incompatible API changes and its release necessitates a Major version bump. label May 24, 2019
@dpatil-magento
Copy link
Contributor

@sirugh I'm bit confused on purpose of new build pipeline. Need to discuss.

@tjwiebell Observations -

  1. Be on page any page other than 1 and reload browser, page 1 is being displayed. Or just try accessing https://venia-8cmy79725.now.sh/venia-bottoms.html?page=4 , instead pf requested page 4, page 1 is being displayed. Page 1 should only display if page 4 does not exists.

  2. Click on sub-category which has just 1 product (Shop the look > retire-your-lbd). For fraction of seconds entire page skeleton shows loading and then 1 product is displayed. I can show this behavior if not well explained here.

  3. Even though category has just 1 product, URL has "page=1" parameter, not sure if its expected? https://venia-8cmy79725.now.sh/shop-the-look/retire-your-lbd.html?page=1

@dpatil-magento dpatil-magento self-assigned this May 27, 2019
@vercel vercel bot temporarily deployed to staging May 28, 2019 22:08 Inactive
@tjwiebell tjwiebell requested a review from sirugh May 29, 2019 13:42
@dpatil-magento
Copy link
Contributor

@tjwiebell QA pass but need final review approval to get merged.

@tjwiebell tjwiebell requested a review from sirugh May 30, 2019 14:04
sirugh
sirugh approved these changes May 30, 2019
View reviewed changes
Copy link
Contributor

@sirugh sirugh left a comment

Choose a reason for hiding this comment

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

@dpatil-magento dpatil-magento merged commit 5db8a50 into develop May 30, 2019
@jimbo jimbo mentioned this pull request Jun 1, 2019
Merged
8 tasks
@supernova-at supernova-at deleted the feature/category-peregrine branch July 25, 2019 19:59
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@sirugh sirugh sirugh approved these changes

@jimbo jimbo jimbo left review comments

@zetlen zetlen Awaiting requested review from zetlen

Assignees

@dpatil-magento dpatil-magento

Labels
version: Major This changeset includes incompatible API changes and its release necessitates a Major version bump.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Peregrine: Category Component
5 participants
@tjwiebell @dpatil-magento @sirugh @zetlen @jimbo