Added real swatch color resolves #1633 #2151

deiserh · 2020-02-08T14:22:25Z

Description

Changed the random color of the swatch to the real one.

Related Issue

Closes #1633 .

Verification Steps

Go to a configurable product.
The colors of the swatches are the real ones.

Checklist

I have updated tests to cover my changes.

Compatibility

With Magento 2.3.5 or higher

PWAStudioBot · 2020-02-08T14:29:58Z

	Messages
📖	Access a deployed version of this PR here. Make sure to wait for the "pwa-pull-request-deploy" job to complete.
📖	DangerCI Failures related to missing labels/description/linked issues/etc will persist until the next push or next nightly build run (assuming they are fixed).

Generated by 🚫 dangerJS against 409787c

deiserh · 2020-02-08T14:44:59Z

What should I do regarding node failed?

larsroettig

@zetlen looks very nice

jimbo · 2020-02-11T19:01:19Z

Awesome. Looks like a good, straightforward implementation.

Does GQL always return a single hex color? Can it return multiple colors or an image?

sirugh · 2020-02-11T19:09:59Z

What should I do regarding node failed?

Run the validate-queries script locally. See if the output indicates the failure reason. If you need help after that, let us know.

That said, it seems like the deployed version does not have a swatch property on graphql. Has that feature been released?

larsroettig · 2020-02-11T20:08:41Z

Awesome. Looks like a good, straightforward implementation.

Does GQL always return a single hex color? Can it return multiple colors or an image?

Hi @jimbo,
your are correct image is allowed see: https://docs.magento.com/m2/ce/user_guide/catalog/swatch-create.html

For Hex color you can only have one by default.

I will test this Ticket tomorrow with an image swatch and update this comment if it is
working.

@jimbo you are correct can be also a string of the image

                {
                  "default_label": "Latte",
                  "label": "Latte",
                  "store_label": "Latte",
                  "use_default_value": true,
                  "value_index": 15,
                  "swatch_data": {
                    "value": "#C0a890"
                  }
                },
                {
                  "default_label": "Jeans",
                  "label": "Jeans",
                  "store_label": "Jeans",
                  "use_default_value": true,
                  "value_index": 66,
                  "swatch_data": {
                    "value": "/j/e/jeans.jpg"
                  }

What I recommend making sure if a picture does not break the logic and create new ticket in the backlog

best regards,

Lars

larsroettig

Image swatch can break this logic see my comment

sirugh

Please try to resolve the error that is thrown on the deployed PR page:

https://pr-2151.pwa-venia.com/venia-dresses.html

…d not shown swatches on the category page

deiserh · 2020-02-18T18:15:31Z

I Updated the Pull Request the error of validate-queries is that some functions are depricated and the Magento Version should be between 2.3.3-2.3.4 but the swatch graphql api was introduced in 2.3.5.

I also added a fallback to empty if no swatch is available.

sirugh · 2020-02-20T21:10:00Z

the swatch graphql api was introduced in 2.3.5.

Ah, there is the problem. We develop against the latest released version ie 2.3.4, so this will cause our CI to start failing if we merge it as is.

This may be a good use case for the new extensibility framework though I don't think we have published our docs for it. If you urgently need this feature in your own line of work I would suggest making the change on your own fork for now.

If you want to see how we implemented page builder as an extension, check out the packages/pagebuilder directory, specifically the intercept file and how it taps into richContentRenderers. I'd love to hear your thoughts on how we might approach implementing a 2.3.5 feature as an extension.

zetlen · 2020-03-02T21:47:19Z

I think this PR is absolutely a candidate for core functionality; it should be our default swatch implementation, instead of the one that uses random colors!

This kind of thing can be implemented as an extension, but I think this changeset in particular should be merged (once 2.3.5 is out).

In the meantime, @deiserh, please maintain this code in your own fork and we'll merge it as soon as our CI is rebased on to 2.3.5.

larsroettig

As mentions from the code side, it looks we need to wait for
2.3.5 should be released on April 28, 2020. I maintain this branch regularly

sirugh

The selected image is broken. On develop the check appears over the color but on this branch the color turns white.

sirugh · 2020-05-05T16:06:48Z

packages/venia-ui/lib/components/ProductOptions/swatch.css

@@ -1,6 +1,6 @@
 .root {
    composes: root from './tile.css';
-    background-color: rgb(var(--venia-swatch-bg));
+    background: var(--venia-swatch-bg);


The background property is shorthand and allows us to use a single variable to provide either the URL or the color.

Signed-off-by: sirugh <rugh@adobe.com>

sirugh · 2020-05-05T16:14:06Z

packages/venia-ui/lib/util/makeUrl.js

@@ -21,7 +21,8 @@ const joinUrls = (base, url) =>

 const mediaBases = new Map()
    .set('image-product', 'catalog/product/')
-    .set('image-category', 'catalog/category/');
+    .set('image-category', 'catalog/category/')
+    .set('image-swatch', 'attribute/swatch');


Is this where swatch images are served? I'd have to test this but I don't have a magento instance configured to try it out. @dpatil-magento do you know how to test this?

I've confirmed on my local that both image swatch value and thumbnail are in this path. value returns a relative path, while thumbnail returns an absolute path (which I think is less fragile to bad config).

tjwiebell

The actual scope of this issue looks perfect 👌

The scope creep of image swatch is my only hang up. I would be willing to accept with some minor tweaks that would only really support square images. HMU if you need help getting your local setup with some swatches!

tjwiebell · 2020-05-05T21:09:12Z

packages/venia-ui/lib/fragments/productDetails.graphql

@@ -49,6 +49,9 @@ fragment ProductDetails on ProductInterface {
                store_label
                use_default_value
                value_index
+                swatch_data {
+                    value


Magento already has an optimized thumbnail which I think would be better to use; this also gives you better indicator that its an image instead of color swatch. Apply same to other query.

Suggested change

value

... on ImageSwatchData {

thumbnail

}

value

tjwiebell · 2020-05-05T21:09:39Z

packages/venia-ui/lib/util/makeUrl.js

@@ -21,7 +21,8 @@ const joinUrls = (base, url) =>

 const mediaBases = new Map()
    .set('image-product', 'catalog/product/')
-    .set('image-category', 'catalog/category/');
+    .set('image-category', 'catalog/category/')
+    .set('image-swatch', 'attribute/swatch');


Suggested change

.set('image-swatch', 'attribute/swatch');

.set('image-swatch', 'attribute/swatch/');

tjwiebell · 2020-05-05T21:11:21Z

packages/venia-ui/lib/util/makeUrl.js

@@ -21,7 +21,8 @@ const joinUrls = (base, url) =>

 const mediaBases = new Map()
    .set('image-product', 'catalog/product/')
-    .set('image-category', 'catalog/category/');
+    .set('image-category', 'catalog/category/')
+    .set('image-swatch', 'attribute/swatch');


I've confirmed on my local that both image swatch value and thumbnail are in this path. value returns a relative path, while thumbnail returns an absolute path (which I think is less fragile to bad config).

tjwiebell · 2020-05-05T21:13:00Z

packages/venia-ui/lib/components/ProductOptions/swatch.js

-    //   https://github.com/magento/pwa-studio/issues/1633
-    const randomColor = memoizedGetRandomColor(value_index);
+    let swatchValue;
+    const isImage = swatch_data && swatch_data.value.charAt(0) !== '#';


Suggested change

const isImage = swatch_data && swatch_data.value.charAt(0) !== '#';

const isImage = swatch_data && swatch_data.thumbnail;

tjwiebell · 2020-05-05T21:36:07Z

packages/venia-ui/lib/components/ProductOptions/swatch.js

+    const isImage = swatch_data && swatch_data.value.charAt(0) !== '#';
+
+    if (isImage) {
+        const imagePath = generateUrlFromContainerWidth(


So, having provided feedback on image swatch, I'm a bit apprehensive to include it in this scope. If I use one of our 4x5 images this is the result as-is:

This is the result if I use the optimized thumbnail from GraphQL:

And finally if I change the implementation to hard-code width/height to the exact dimensions of these tiles (3rem/3rem or 48px/48px):

We're making progress on not assuming every images aspect ratio, but a square box now forces us to actually support this natively. Would like the team's thoughts on this; but I'm going to propose we remove support for image swatch until we can support different aspect ratios.

I would be willing to accept this if we put in a comment that says you must upload swatch images with a 1x1 aspect ratio, and change implementation to this (only way I could get images to render correctly):

const imagePath = generateUrl(swatch_data.thumbnail, 'image-swatch')(48);

I'm going to propose we remove support for image swatch until we can support different aspect ratios.

I think that is fair. The initial scope of this ticket was to replace random color with the actual color.

However it does seem like swatch_data.value can be an image url. We still need to have a fall-back thing to display.

sirugh · 2020-05-06T15:20:10Z

Upon further review it looks like this needs some more work to get the color filter swatches and the product edit swatches functional.

sirugh · 2020-05-06T15:20:57Z

That said, we may have to remove the filter swatches because the aggregation response from the server does not include color values:

…ault background color.

…ons from graphql do not include swatch values Signed-off-by: sirugh <rugh@adobe.com>

tjwiebell

Minor cleanup since you moved some logic around, but code works 👍

One small problem found testing; the selected state for images doesn't overlay the same as color. Please investigate.

Unselected:

Selected:

tjwiebell · 2020-05-06T19:50:12Z

packages/venia-ui/lib/components/ProductOptions/swatch.js

@@ -21,6 +21,9 @@ import { useSwatch } from '@magento/peregrine/lib/talons/ProductOptions/useSwatc
 const getClassName = (name, isSelected, hasFocus) =>
    `${name}${isSelected ? '_selected' : ''}${hasFocus ? '_focused' : ''}`;

+// Swatches _must_ have a 1x1 aspect ratio to match the UI.


Man, it'd be real cool if comments supported markdown 😂

tjwiebell · 2020-05-06T19:52:47Z

packages/venia-ui/lib/components/ProductOptions/swatch.js

-    } else {
-        swatchValue = swatch_data.value;
+    let finalStyle = style;
+    let swatchValue = '';


Move to inside the scope of if (swatch_data)

tjwiebell · 2020-05-06T20:00:03Z

packages/venia-ui/lib/components/ProductOptions/swatch.js

+    let finalStyle = style;
+    let swatchValue = '';
+    if (swatch_data) {
+        const isImage = swatch_data && swatch_data.thumbnail;


Nitpicky cleanup since you wrapped in if, sorry 😬I would almost try to simplify init of swatchValue to ternary if it doesn't look too gnarly.

Suggested change

const isImage = swatch_data && swatch_data.thumbnail;

const {thumbnail, value} = swatch_data;

if (thumbnail) { ... }

Yea I can clean this up :)

Err swatch_data could be undefined so I'll just try to handle this gracefully.

Resolved in 69386ab

Signed-off-by: sirugh <rugh@adobe.com>

tjwiebell

Nicely done, looks great! And thank you @deiserh for the original contribution, sorry for the delay 🙂

updated

deiserh · 2020-05-08T08:56:14Z

Thank you for finalizing the pr 😁

magento-engcom-team added Partner: TechDivision partners-contribution labels Feb 8, 2020

m2-community-project bot added this to Ready for Review in Pull Request Progress Feb 8, 2020

PWAStudioBot added the pkg:venia-ui label Feb 8, 2020

Added swatch color resolves magento#1633

4333d80

deiserh force-pushed the feature/#1633 branch from 6f09c61 to 4333d80 Compare February 8, 2020 14:35

larsroettig approved these changes Feb 8, 2020

View reviewed changes

m2-community-project bot assigned larsroettig Feb 8, 2020

m2-community-project bot moved this from Ready for Review to Reviewer Approved in Pull Request Progress Feb 8, 2020

m2-community-project bot added the Progress: approved label Feb 8, 2020

larsroettig suggested changes Feb 16, 2020

View reviewed changes

sirugh requested changes Feb 18, 2020

View reviewed changes

m2-community-project bot assigned sirugh Feb 18, 2020

m2-community-project bot moved this from Reviewer Approved to Changes Requested in Pull Request Progress Feb 18, 2020

Added the fix to show images and added a empty fallback value to avoi…

39200dd

…d not shown swatches on the category page

Merge branch 'develop' into feature/magento#1633

8209d9e

larsroettig previously approved these changes Mar 31, 2020

View reviewed changes

sirugh added the hold On hold until another condition is fulfilled. label Apr 20, 2020

Merge branch 'develop' into feature/magento#1633

4c4d11a

larsroettig removed the hold On hold until another condition is fulfilled. label Apr 30, 2020

Merge branch 'develop' into feature/magento#1633

a63c9df

sirugh previously requested changes May 4, 2020

View reviewed changes

sirugh dismissed larsroettig’s stale review via 3224c5f May 5, 2020 16:05

sirugh reviewed May 5, 2020

View reviewed changes

Uses background css property to set either color or image

87813ca

Signed-off-by: sirugh <rugh@adobe.com>

sirugh force-pushed the feature/#1633 branch from 3224c5f to 87813ca Compare May 5, 2020 16:11

sirugh reviewed May 5, 2020

View reviewed changes

tjwiebell suggested changes May 5, 2020

View reviewed changes

m2-community-project bot assigned tjwiebell May 5, 2020

m2-community-project bot added Progress: changes requested and removed Progress: approved labels May 5, 2020

sirugh added 2 commits May 6, 2020 11:00

Use thumbnail as image if provided, else use color or fallback to def…

a6015d1

…ault background color.

Removes fashion color swatch from filter drawer as possible aggregati…

5b734bb

…ons from graphql do not include swatch values Signed-off-by: sirugh <rugh@adobe.com>

PWAStudioBot added the pkg:peregrine label May 6, 2020

tjwiebell added the version: Minor This changeset includes functionality added in a backwards compatible manner. label May 6, 2020

tjwiebell suggested changes May 6, 2020

View reviewed changes

Fixes selected image overlay and other feedback

69386ab

Signed-off-by: sirugh <rugh@adobe.com>

tjwiebell approved these changes May 7, 2020

View reviewed changes

Merge branch 'develop' into feature/magento#1633

409787c

dpatil-magento requested a review from sirugh May 7, 2020 18:31

dpatil-magento merged commit 76cf84b into magento:develop May 7, 2020

m2-community-project bot moved this from Changes Requested to Done in Pull Request Progress May 7, 2020

m2-community-project bot removed the Progress: changes requested label May 7, 2020

	.set('image-swatch', 'attribute/swatch');
	.set('image-swatch', 'attribute/swatch/');

	const isImage = swatch_data && swatch_data.value.charAt(0) !== '#';
	const isImage = swatch_data && swatch_data.thumbnail;

	const isImage = swatch_data && swatch_data.thumbnail;
	const {thumbnail, value} = swatch_data;
	if (thumbnail) { ... }

Added real swatch color resolves #1633 #2151

Added real swatch color resolves #1633 #2151

Conversation

deiserh commented Feb 8, 2020 • edited

Description

Related Issue

Verification Steps

Checklist

Compatibility

PWAStudioBot commented Feb 8, 2020 • edited

deiserh commented Feb 8, 2020

larsroettig left a comment

Choose a reason for hiding this comment

jimbo commented Feb 11, 2020

sirugh commented Feb 11, 2020 • edited

larsroettig commented Feb 11, 2020 • edited

larsroettig left a comment

Choose a reason for hiding this comment

sirugh left a comment

Choose a reason for hiding this comment

deiserh commented Feb 18, 2020 • edited

sirugh commented Feb 20, 2020

zetlen commented Mar 2, 2020

larsroettig left a comment • edited

Choose a reason for hiding this comment

sirugh left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tjwiebell left a comment

Choose a reason for hiding this comment

tjwiebell May 5, 2020 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tjwiebell May 5, 2020 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sirugh commented May 6, 2020

sirugh commented May 6, 2020

tjwiebell left a comment • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tjwiebell left a comment

Choose a reason for hiding this comment

deiserh commented May 8, 2020

deiserh commented Feb 8, 2020 •

edited

PWAStudioBot commented Feb 8, 2020 •

edited

sirugh commented Feb 11, 2020 •

edited

larsroettig commented Feb 11, 2020 •

edited

deiserh commented Feb 18, 2020 •

edited

larsroettig left a comment •

edited

tjwiebell May 5, 2020 •

edited

tjwiebell May 5, 2020 •

edited

tjwiebell left a comment •

edited