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

Wishlist #2030

Merged
merged 27 commits into from Aug 23, 2022
Merged

Wishlist #2030

merged 27 commits into from Aug 23, 2022

Conversation

IvanJavorovic
Copy link
Contributor

Q A
Bug fix? no
New feature? yes
BC breaks? no
Deprecations? no

WIP new Wishlist bundle added to replace existing wishlist functionality. Review and changes are needed.

@CLAassistant
Copy link

CLAassistant commented Jun 27, 2022

CLA assistant check
All committers have signed the CLA.

@IvanJavorovic IvanJavorovic marked this pull request as ready for review June 27, 2022 11:48
@dkarlovi
Copy link
Contributor

dkarlovi commented Jul 5, 2022

Looking at the PR, it's very nice progress. I think the direction is good, but since the change set is so large, it seems to me there's a lot of similarities with Cart that we should be able to reuse more code maybe?

For example, let's say we introduce a "product comparison" feature in the future, it would also be a list of products which you can add/remove to a separate storage, it would seem wasteful to need to again add so much functionality. With this in mind, is there anything we could do to abstract code for better reuse here?

@dpfaffenbauer
Copy link
Member

@dkarlovi Absolutely! The plan was to first create the wishlist and check what we need, and then further refactor and abstract it. Stuff could move into the StorageList Component and maybe we also have to create a StorageListBundle to abstract and combine stuff. But I am looking into that

@dpfaffenbauer
Copy link
Member

@dkarlovi reworked a lot now to share more between Order and Wishlist, you might wanna check?

@dpfaffenbauer dpfaffenbauer merged commit 71fa4c3 into coreshop:master Aug 23, 2022
@dpfaffenbauer
Copy link
Member

thanks @IvanJavorovic for kicking this off

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants