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

Fix exporting patterns with transparent color #6318

Merged
merged 8 commits into from
Sep 22, 2022
Merged

Conversation

archmoj
Copy link
Contributor

@archmoj archmoj commented Sep 19, 2022

@plotly/plotly_js

@archmoj archmoj added bug something broken status: reviewable labels Sep 19, 2022
@alexcjohnson
Copy link
Collaborator

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

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:

@archmoj
Copy link
Contributor Author

archmoj commented Sep 19, 2022

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

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:

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

@archmoj archmoj changed the title Fix pattern SVG drawings using stroke-opacity Fix exporting patterns with transparent color Sep 19, 2022
@alexcjohnson
Copy link
Collaborator

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 #6318 (comment)

@archmoj
Copy link
Contributor Author

archmoj commented Sep 19, 2022

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
Copy link
Contributor Author

archmoj commented Sep 19, 2022

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?

@alexcjohnson
Copy link
Collaborator

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.

@archmoj
Copy link
Contributor Author

archmoj commented Sep 19, 2022

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

Good call. Addressed in 4cd8011.

@archmoj
Copy link
Contributor Author

archmoj commented Sep 21, 2022

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

Copy link
Collaborator

@alexcjohnson alexcjohnson left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💃 Yep, looks great!

@archmoj archmoj merged commit 3bab776 into master Sep 22, 2022
@archmoj archmoj deleted the pattern-stroke-opacity branch September 22, 2022 13:02
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug something broken
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants