Skip to content

Conversation

@1070rik
Copy link
Contributor

@1070rik 1070rik commented Jan 23, 2020

Short Description and Why It's Useful

The old function wouldn't show any categories at all if you added the "*.name" field in the includedFields array in your config. The way it works now is also easier to read.

Screenshots of Visual Changes before/after (if There Are Any)

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 Vue Storefront sites with this new feature

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

Contribution and Currently Important Rules Acceptance

@1070rik 1070rik requested a review from andrzejewsky January 23, 2020 12:25
for (let subcat of category.children_data) {
if (subcat.name) {
return slugifyCategories({ ...subcat, slug: createSlug(subcat) } as any as ChildrenData)
category.children_data.forEach((subCat: ChildrenData): void => {
Copy link
Contributor

@andrzejewsky andrzejewsky Jan 28, 2020

Choose a reason for hiding this comment

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

@1070rik What about shomething like this (simpler)?

category.children_data = category.children_data.map(ch => ({
  ...ch,
  ...(ch.name ? { slug: createSlug(subCat) } : {})
}))

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Although I don't really see the added value of doing this instead of a forEach, this would only work for the first 2 levels of categories.
The slugifyCategories(subCat) call at the end of the loop would cause a recursive loop which would give the subcategories of that subcategory a slug.

Copy link
Contributor

Choose a reason for hiding this comment

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

@1070rik yes, but I didn't mention about deleting slugifyCategories(subCat) my changes are related to only for, it's just simpler, and actually entire function could be refactored with using map call. However - it's your choice, just let me know if you want to spare time on it or not - it's still good PR (it solves the issue, thanks!)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Turning it into a map and doing the function call doesn't really make the code look better or easier, it only makes it more complex in my opinion. I still prefer the forEach method over the map since it easier to understand.

Copy link
Contributor

Choose a reason for hiding this comment

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

for me, it looks much better, it's just imperative vs functional. In this case, it maybe doesn't change so much, but remember that in the for you are mutating values while map returns a new array - no side effects (they could be painful sometimes). As I mentioned entire function could be refactored using map - instead of modifying category you can return a new one with slugs inside (again, no side effects).

Of course, this solution is also fine, thanks!

@andrzejewsky andrzejewsky self-requested a review January 29, 2020 09:15
@andrzejewsky andrzejewsky merged commit 234da12 into vuestorefront:develop Jan 29, 2020
@1070rik 1070rik deleted the bugfix/child-categories-slug branch January 29, 2020 13:03
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.

2 participants