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

Color and texture consistent when both are set #15284

Merged
merged 6 commits into from Jan 10, 2020

Conversation

@davidglezz
Copy link
Contributor

davidglezz commented Aug 28, 2019

Questions Answers
Branch? develop
Description? Product variants may be color or texture but not both.
This refacto changes 2 if by if-elseif
Type? refacto
Category? FO
BC breaks? no
Deprecations? no
Fixed ticket? no
How to test? Check that the product variants are shown correctly in:
Product with color and texture variants in Product miniature, product page and quick view.

Product variants may be color or texture but not both.
As seen in
https://github.com/PrestaShop/PrestaShop/pull/12523/files#diff-284fd264950a297a1db191d6d0946ef9R47
and
https://github.com/PrestaShop/PrestaShop/blob/develop/themes/classic/templates/catalog/_partials/facets.tpl#L85

I used if-elseif instead to improve readability and not see that both could be simultaneously.


This change is Reviewable

Product variants may be color or texture but not both.
As seen in
https://github.com/PrestaShop/PrestaShop/pull/12523/files#diff-284fd264950a297a1db191d6d0946ef9R47
and
https://github.com/PrestaShop/PrestaShop/blob/develop/themes/classic/templates/catalog/_partials/facets.tpl#L85

I used if-elseif instead to improve readability and not see that both could be simultaneously.
@davidglezz davidglezz requested a review from PrestaShop/prestashop-core-developers as a code owner Aug 28, 2019
@marionf

This comment has been minimized.

Copy link
Contributor

marionf commented Aug 28, 2019

@kpodemski What do you think of this change ?

@PierreRambaud

This comment has been minimized.

Copy link
Contributor

PierreRambaud commented Aug 28, 2019

Seems legit. By definition we can't have both.

@marionf

This comment has been minimized.

Copy link
Contributor

marionf commented Aug 28, 2019

@kpodemski would like to have both: #10596 (comment)

@PierreRambaud

This comment has been minimized.

Copy link
Contributor

PierreRambaud commented Aug 28, 2019

This is what this commit is doing :D
If texture => use texture
Else if color => use color

But you're still able to save both data in the BO. But you can't render both in FO.

@kpodemski

This comment has been minimized.

Copy link
Contributor

kpodemski commented Aug 28, 2019

👍

@marionf

This comment has been minimized.

Copy link
Contributor

marionf commented Sep 4, 2019

@davidglezz

This PR doesn't change the current behavior which is:

If I add a texture AND a color:

capture d'écran_1940

  • Attribute listing BO = texture

capture d'écran_1941

  • Attribute listing product page BO = color

capture d'écran_1942

  • Product page FO = texture

capture d'écran_1943

  • Product listing FO = color

capture d'écran_1944

  • Quickview FO = texture

capture d'écran_1945

@PierreRambaud

This comment has been minimized.

Copy link
Contributor

PierreRambaud commented Oct 2, 2019

Hi @davidglezz Your pull request needs to be rebased to continue :)

@davidglezz

This comment has been minimized.

Copy link
Contributor Author

davidglezz commented Oct 4, 2019

The original intention of this PR was to improve and simplify the "if" condition of the template "product-variants.tpl", but in the end I modified the rest of the templates to be consistent:

If texture => use texture
Else if color => use color

On the other hand, I have not been able to perform my own tests (problems with the current dockerfile) but the screenshots provided by @marionf in "Product listing FO = color" do not fit with the code.

I will investigate a bit more and try to provide screenshots, apologies for the delay, I have little time.

@davidglezz

This comment has been minimized.

Copy link
Contributor Author

davidglezz commented Dec 19, 2019

This PR makes color and texture consistent in all places.

Previously, if both color and texture were selected, then in some places color was shown and in others texture was shown.

If the 2 are set, texture is shown. If the user wants to show the color it will be easy to delete the texture.

If I add a texture AND a color:
image

  • Attribute listing BO = texture
    image

  • Attribute listing product page BO = texture
    image

  • Product page FO and Quickview FO = texture
    image

  • Product listing FO = texture
    image

@davidglezz davidglezz changed the title Make color and texture exclusive (no both) Color and texture consistent when both are set Dec 19, 2019
@marionf marionf removed their assignment Dec 27, 2019
@sarahdib sarahdib added QA ✔️ and removed waiting for QA labels Jan 10, 2020
@sarahdib sarahdib added this to the 1.7.7.0 milestone Jan 10, 2020
@Progi1984 Progi1984 merged commit d00f5de into PrestaShop:develop Jan 10, 2020
2 checks passed
2 checks passed
PrettyCI Code formatting
Details
Travis CI - Pull Request Build Passed
Details
@Progi1984

This comment has been minimized.

Copy link
Contributor

Progi1984 commented Jan 10, 2020

Thanks @davidglezz

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
7 participants
You can’t perform that action at this time.