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

Verify products #73

Closed
wants to merge 7 commits into from
Closed

Conversation

LukasOcy
Copy link

  • Add property visibility to comparison
  • Verify product ids that are saved to locally and presented on the compare button. If a product is not available anymore, it should not be considered for comparison and therefore the number of products should be displayed correctly.

@LukasOcy
Copy link
Author

@tinect Do you know, why the pipeline fails? What can I do to fix it?

@tinect
Copy link
Member

tinect commented Jun 10, 2024

Hi @LukasOcy

The issue seems shopware related. I'll try to find something out within the next days.

Is it worth doing extra request and loading entire products just to check for inactive product on every page visit?

@LukasOcy
Copy link
Author

Hi @tinect

Thank you. Well, it is a bit confusing for the customer when clicking a button that says 4 products and then the off canvas comparison list is empty. On the other hand you are right, it is extra effort every time the page is loaded. Do you have an alternative idea?

@tinect
Copy link
Member

tinect commented Jun 11, 2024

I would just check it when the offcanvas or comparation is started and check the incoming productsIds count. If it's not the same, print a message, like X product/s are not available and has/have been removed from your list.

@LukasOcy
Copy link
Author

@tinect I had a discussion with my team and we think it is a good idea to check it when the page is loaded. The effort to do that is not huge, so the performance is not effected. Also, if we check it when the off canvas list is opened, we still need to adjust the local storage, otherwise the missing products would never been removed. Could you please accept the PR? Thanks.

@tinect
Copy link
Member

tinect commented Jun 11, 2024

Hmm, I don't see it this way, while there is not just one more request on every page load, but also loading the entire products, too - instead of just checking their existence.

I'd prefer you decorate the needed services on your project.
What do you think @vienthuong @shyim ?

@tinect
Copy link
Member

tinect commented Jun 18, 2024

Thank you for your suggestion. After internal discussions, we've decided not to implement this feature for various reasons. However, if you need it, you are welcome to incorporate it into your project.

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