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

#3286 + #3332 catalog-next module improvements #3295

Merged

Conversation

cewald
Copy link
Contributor

@cewald cewald commented Jul 26, 2019

Related issues

closes #3286 and #3332

Short description and why it's useful

I added a new state item, containing the current category id, to be able to traverse the current category even if the current route path is not the same as in url_path nor slug (e.g. if you use rewrites). I decided to use just the id in currentId state property to prevent duplicate content in the state, because the category is already in categoriesMap.

Also I changed how the filter for the category is added to the action in the category page. This way you don't need to ask for config.products.useMagentoUrlKeys and use just the given parameters of the current route. This is more specific and also more flexible than just get it by the path.

And I fixed the bug mentioned in #3286 where it sometimes returns the wrong category because of the flacky include()-search in the getCategoryFrom() getter.

Which environment this relates to

Check your case. In case of any doubts please read about Release Cycle

  • Test version (https://test.storefrontcloud.io) - this is a new feature or improvement for Vue Storefront. I've created branch from develop branch and want to merge it back to develop
  • RC version (https://next.storefrontcloud.io) - this is a stabilisation fix for Release Candidate of Vue Storefront. I've created branch from release branch and want to merge it back to release
  • Stable version (https://demo.storefrontcloud.io) - this is an important fix for current stable version. I've created branch from hotfix or master branch and want to merge it back to hotfix

Upgrade Notes and Changelog

  • No upgrade steps required (100% backward compatibility and no breaking changes)
  • I've updated the Upgrade notes and Changelog on how to port existing VS sites with this new feature

IMPORTANT NOTICE - Remember to update CHANGELOG.md with description of your change

Contribution and currently important rules acceptance

cewald referenced this pull request in icmaa/vue-storefront Jul 26, 2019
… module extension and implement it into our modules

* @todo: remove module-extension when #3295 is approved and merged
cewald referenced this pull request in icmaa/vue-storefront Jul 26, 2019
* Change `category-next/getCurrentCategory` as module extension and implement it into our modules
* @todo: remove module-extension when #3295 is approved and merged
@lukeromanowicz
Copy link
Contributor

Looks good, awesome job 👍

@pkarw pkarw requested a review from patzick July 29, 2019 06:30
@patzick
Copy link
Collaborator

patzick commented Jul 29, 2019

Thanks @cewald, i think it's better to improve category match pattern instead of extending state to store categoryId. Removing it was a purpose to have single source of truth in url without need to set it maunually.
Let's hold this one to rethink it :)

@patzick patzick added the not ready for merge PR is holded. Needs some clarifications or things that need to be finished. label Jul 29, 2019
@cewald
Copy link
Contributor Author

cewald commented Jul 30, 2019

@patzick I totally like this approach of having a "true" single source of truth, thats why I just added the ID instead of a own category object, but yes indeed it would be better to get that without a extra state entry. But how is it then possible to see if this url-key is probably connected to a custom route if I don't use the url-key in category-state?

For example I added a route like via a custom url mappingFallback() rule like:

{
  "name": "category",
  "params": {
    "slug": "new"
  }
}

A use-case for that is to have custom url's for a specific standard route. For example: our marketing want's an url that isn't conform with our default url-category pattern but then again sounds better in communication.

@cewald
Copy link
Contributor Author

cewald commented Jul 31, 2019

@patzick I updated my pull request as you said. I found a way to traverse the current url in a better way without the need of an extra current category state. I was able to get the current category in a more specific way than using url-key, by using the route.params (like in URL mapping). This way it's possible to have any url-key you want and still find the current category by it's original url-key from route.params.

I put it into a new method so we still have both ways – there are use-cases when it's enough to look just after the url-key.

@cewald cewald changed the title #3286 catalog-next module improvements #3286 + #3332 catalog-next module improvements Aug 5, 2019
@pkarw
Copy link
Collaborator

pkarw commented Aug 12, 2019

@cewald is this one ready to be merged in?

@pkarw
Copy link
Collaborator

pkarw commented Aug 12, 2019

awesome job by the way @cewald :-)

@pkarw pkarw added QA - Ready for tests This is notification for testers, that improvement is ready to be tested and verified. and removed not ready for merge PR is holded. Needs some clarifications or things that need to be finished. labels Aug 12, 2019
Copy link
Collaborator

@pkarw pkarw left a comment

Choose a reason for hiding this comment

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

Just a very very small change suggested; overall: great job

core/modules/catalog-next/helpers/categoryHelpers.ts Outdated Show resolved Hide resolved
@pkarw
Copy link
Collaborator

pkarw commented Aug 12, 2019

Thasnk

})
state.categoriesMap = Object.assign(state.categoriesMap, newCategoriesEntry)
Copy link
Collaborator

Choose a reason for hiding this comment

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

i'll only change that back for one reason - with every category vue will mutate state and will invoke recalculate all computed properties and getters which are based on this. Fix for reactivity here is actually change
state.categoriesMap = Object.assign(state.categoriesMap, newCategoriesEntry)
to
state.categoriesMap = Object.assign({}, state.categoriesMap, newCategoriesEntry)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ah nice, didn't figured that out :)

Copy link
Collaborator

@patzick patzick left a comment

Choose a reason for hiding this comment

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

Great work @cewald , i'll only change back CATEGORY_ADD_CATEGORIES to invoke state mutation once, but overall really great job! :)

@patzick patzick requested a review from pkarw August 20, 2019 14:17
@patzick patzick merged commit a1501f1 into vuestorefront:develop Aug 20, 2019
@patzick patzick removed the QA - Ready for tests This is notification for testers, that improvement is ready to be tested and verified. label Aug 20, 2019
@patzick patzick mentioned this pull request Aug 20, 2019
3 tasks
@cewald cewald deleted the bugfix/3286-catalog-next-improvements branch August 22, 2019 05:40
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
vs-hackathon Tasks for the Hackathon
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants