plotly.js icon indicating copy to clipboard operation
plotly.js copied to clipboard

Published 20 hours ago • verified publisher icon plotly

Fix exporting patterns with transparent color

Open archmoj opened this issue 3 years ago • 8 comments
trafficstars

@plotly/plotly_js

archmoj avatar Sep 19 '22 15:09 archmoj

Looks like pattern . needs the same applied to fill (ie create fill-opacity) instead of stroke

https://github.com/plotly/plotly.js/blob/adcb081a09fec15e7af2d70210e01a525b83cacd/src/components/drawing/index.js#L530

But I wonder: is there any difference in effect between opacity and stroke-opacity (or fill-opacity) in this case? ie could we just multiply fgopacity and fgStrokeOpacity together into a single opacity value?

Also: looks like the bgcolor part may need to be split into fill and fill-opacity, since we aren't setting it with Color.fill:

https://github.com/plotly/plotly.js/blob/adcb081a09fec15e7af2d70210e01a525b83cacd/src/components/drawing/index.js#L571

alexcjohnson avatar Sep 19 '22 16:09 alexcjohnson

Looks like pattern . needs the same applied to fill (ie create fill-opacity) instead of stroke

https://github.com/plotly/plotly.js/blob/adcb081a09fec15e7af2d70210e01a525b83cacd/src/components/drawing/index.js#L530

But I wonder: is there any difference in effect between opacity and stroke-opacity (or fill-opacity) in this case? ie could we just multiply fgopacity and fgStrokeOpacity together into a single opacity value?

Also: looks like the bgcolor part may need to be split into fill and fill-opacity, since we aren't setting it with Color.fill:

https://github.com/plotly/plotly.js/blob/adcb081a09fec15e7af2d70210e01a525b83cacd/src/components/drawing/index.js#L571

Browser compatibility of stroke-opacity and fill-opacity is unknown. So good call. Please review 22dbf02 and 35f7e95.

archmoj avatar Sep 19 '22 19:09 archmoj

Browser compatibility of stroke-opacity and fill-opacity is unknown.

We've been using those attributes all over the place for years, I think we can conclude they're safe :) But simpler is better as long as the effect is the same. Did anything actually change in the images you regenerated? They look identical to me.

We still want to separate out bgcolor per the last part of my comment https://github.com/plotly/plotly.js/pull/6318#issuecomment-1251242102

alexcjohnson avatar Sep 19 '22 19:09 alexcjohnson

Did anything actually change in the images you regenerated? They look identical to me.

Please see the diffs below:

diff-pattern_bars diff-scatter_fill_pattern

BTW, they look identical to my eyes too.

archmoj avatar Sep 19 '22 19:09 archmoj

Browser compatibility of stroke-opacity and fill-opacity is unknown.

We've been using those attributes all over the place for years, I think we can conclude they're safe :) But simpler is better as long as the effect is the same.

With that said, do you think it would be better (& more specific) to use stroke-opacity and fill-opacity instead of general opacity?

archmoj avatar Sep 19 '22 19:09 archmoj

With that said, do you think it would be better (& more specific) to use stroke-opacity and fill-opacity instead of general opacity?

You could also say opacity is better because it's the same attribute for both types of pattern. I don't think it matters. Let's just fix the bgcolor part and call it done.

alexcjohnson avatar Sep 19 '22 20:09 alexcjohnson

We still want to separate out bgcolor per the last part of my comment #6318 (comment)

Good call. Addressed in 4cd8011.

archmoj avatar Sep 19 '22 20:09 archmoj

@alexcjohnson Let's merge this PR so I possibly open another one for few other components & traces. Thanks!

archmoj avatar Sep 21 '22 00:09 archmoj