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

Paste product combination tokens and highlight invalid tokens #18095

Merged
merged 18 commits into from
Apr 17, 2020

Conversation

MarkALeonard
Copy link
Contributor

@MarkALeonard MarkALeonard commented Mar 11, 2020

Questions Answers
Branch? develop
Description? Allow product combination tokens to be pasted. Pasted comma separated text will be validated and tokens added. Invalid tokens will be coloured yellow. Duplicate tokens will not be added.
Type? improvement
Category? BO
BC breaks? no
Deprecations? no
Fixed ticket? Fixes #{issue number here}.
How to test? On Current Product Page: Copy and paste 'Color : Red, Color : Blue, Color : Red, Color : Green, Color : Invalid' into the product combination tokens field. All tokens will be created including duplicates but no attributes will be selected and pressing the generate button produces a single product combination. On Product Page with this Improvement: Copy and paste 'Color : Red, Color : Blue, Color : Red, Color : Green, Color : Invalid' into the product combination tokens field. Tokens will be created, duplicate tokens will be removed, attributes will be selected and invalid tokens highlighted in yellow. Pressing the generate button will create the correct number of product combinations.

This change is Reviewable

@MarkALeonard MarkALeonard requested a review from a team as a code owner March 11, 2020 14:54
@prestonBot
Copy link
Collaborator

Hello @MarkALeonard!

This is your first pull request on the PrestaShop project. Thank you, and welcome to this Open Source community!

@prestonBot prestonBot added develop Branch Improvement Type: Improvement labels Mar 11, 2020
admin-dev/themes/default/js/bundle/product/form.js Outdated Show resolved Hide resolved
admin-dev/themes/default/js/bundle/product/form.js Outdated Show resolved Hide resolved
admin-dev/themes/default/js/bundle/product/form.js Outdated Show resolved Hide resolved
@NeOMakinG
Copy link

Thanks for your contribution @MarkALeonard, I left some comments mainly aiming good practices :)

MarkALeonard and others added 3 commits March 13, 2020 09:20
Co-Authored-By: SZCZUPAK Valentin <valentin.szczupak@prestashop.com>
Co-Authored-By: SZCZUPAK Valentin <valentin.szczupak@prestashop.com>
Co-Authored-By: SZCZUPAK Valentin <valentin.szczupak@prestashop.com>
@MarkALeonard
Copy link
Contributor Author

Thanks @NeOMakinG for your feedback, I have made the changes accordingly.

@NeOMakinG
Copy link

Thanks @NeOMakinG for your feedback, I have made the changes accordingly.

Did you test it after changes ? Hope the if (attr) is fine in most case, thanks for you work.

@NeOMakinG NeOMakinG requested a review from a team March 13, 2020 09:34
NeOMakinG
NeOMakinG previously approved these changes Mar 13, 2020
@MarkALeonard
Copy link
Contributor Author

Yes, tested and works as expected. Thank you.

@NeOMakinG NeOMakinG requested a review from a team March 13, 2020 09:46
Imorove multiple engine search results.
@MarkALeonard
Copy link
Contributor Author

MarkALeonard commented Mar 13, 2020

On implementing on a live site I noticed the Bloodhound engine returns multiple results for 'Size : S' but not 'Size : M'. Have changed result.length == 1 to result.length >= 1 and first result in array is used. Also fixed issue from original code that reported an error when removing invalid tokens. As the new code has the 'invalid' class I have added a check for that. Apologies for not noticing this initially.

Progi1984
Progi1984 previously approved these changes Mar 13, 2020
Co-Authored-By: Progi1984 <progi1984@gmail.com>
Progi1984
Progi1984 previously approved these changes Mar 13, 2020
admin-dev/themes/default/js/bundle/product/form.js Outdated Show resolved Hide resolved
admin-dev/themes/default/js/bundle/product/form.js Outdated Show resolved Hide resolved
admin-dev/themes/default/js/bundle/product/form.js Outdated Show resolved Hide resolved
Co-Authored-By: SZCZUPAK Valentin <valentin.szczupak@prestashop.com>
Co-Authored-By: GoT <PierreRambaud@users.noreply.github.com>
@MarkALeonard MarkALeonard dismissed stale reviews from NeOMakinG and Progi1984 via 9dbd096 March 17, 2020 09:40
MarkALeonard and others added 4 commits March 17, 2020 09:40
Co-Authored-By: GoT <PierreRambaud@users.noreply.github.com>
Co-Authored-By: GoT <PierreRambaud@users.noreply.github.com>
Co-Authored-By: GoT <PierreRambaud@users.noreply.github.com>
@khouloudbelguith
Copy link
Contributor

Hi @MarkALeonard,

Can you please resolve conflicts.

Thanks!

@khouloudbelguith khouloudbelguith added Waiting for author Status: action required, waiting for author feedback Waiting for dev Status: action required, waiting for tech feedback and removed Waiting for QA Status: action required, waiting for test feedback labels Apr 10, 2020
@Progi1984 Progi1984 removed Waiting for author Status: action required, waiting for author feedback Waiting for dev Status: action required, waiting for tech feedback labels Apr 14, 2020
@Progi1984 Progi1984 added the Waiting for QA Status: action required, waiting for test feedback label Apr 14, 2020
@SD1982 SD1982 self-assigned this Apr 16, 2020
@SD1982 SD1982 added QA ✔️ Status: check done, code approved and removed Waiting for QA Status: action required, waiting for test feedback labels Apr 17, 2020
@SD1982 SD1982 added this to the 1.7.8.0 milestone Apr 17, 2020
@SD1982
Copy link
Contributor

SD1982 commented Apr 17, 2020

LGTM Thanks @MarkALeonard

@Progi1984 Progi1984 merged commit 5db1525 into PrestaShop:develop Apr 17, 2020
@Progi1984
Copy link
Member

Thanks @MarkALeonard

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
develop Branch Improvement Type: Improvement QA ✔️ Status: check done, code approved
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants