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.

Sign up for GitHub

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

Jump to bottom

Implement fgopacity, fgcolor & "overlay" fillmode for bars, fix bar legend and pattern with marker colorscale #5733

Merged
merged 12 commits into from Jun 18, 2021
Merged

Implement fgopacity, fgcolor & "overlay" fillmode for bars, fix bar legend and pattern with marker colorscale #5733

merged 12 commits into from Jun 18, 2021

Conversation

archmoj
Copy link
Contributor

@archmoj archmoj commented Jun 16, 2021

Resolves #5728 and fixes #5285.
Also fixes drawing bar pattern when the marker get a colorscale.

@plotly/plotly_js

@archmoj archmoj added this to the v2.1.0 milestone Jun 16, 2021
@nicolaskruchten
Copy link
Member

Behaviour looks pretty good at first glance! Will dig more tomorrow but nice job :)

@nicolaskruchten
Copy link
Member

The legend items get hard to identify when the size goes up... could we offset the "+" and "." patterns so they're centered?

@nicolaskruchten
Copy link
Member

I'm thinking that we need to tune things a little bit because for the same size/solidity, the different patterns feel quite different to my eye.

Also I'm thinking we need to either make the overlay semi-transparent or add pattern.opacity to make the patterns vibrate a little less strongly IMO.

@nicolaskruchten
Copy link
Member

I'm thinking that we need to tune things a little bit because for the same size/solidity, the different patterns feel quite different to my eye.

Well, playing with it a bit I can see this will be a rabbit-hole so let's leave this alone for now :)

so my asks are:

  1. Can we fix the legend items for + and . patterns
  2. Can we add pattern.opacity and set it to like 0.8 when fillmode=overlay, to make things a bit less intense?

@archmoj
Copy link
Contributor Author

archmoj commented Jun 17, 2021

  1. Can we add pattern.opacity and set it to like 0.8 when fillmode=overlay, to make things a bit less intense?

What about pattern.fgopacity which won't impact the background opacity?

@nicolaskruchten
Copy link
Member

sounds good.

@archmoj archmoj changed the title Implement fgcolor & "overlay" fillmode for bars, fix bar legend and pattern with marker colorscale Implement fgopacity, fgcolor & "overlay" fillmode for bars, fix bar legend and pattern with marker colorscale Jun 17, 2021
@nicolaskruchten
Copy link
Member

oooh that's nice:

image

@archmoj
Copy link
Contributor Author

archmoj commented Jun 17, 2021

  1. Can we fix the legend items for + and . patterns

Good call. The scale for all shapes of legends is now reduced by fd89e23.

@nicolaskruchten
Copy link
Member

OK, this behaviour is all good for me 🌟

@alexcjohnson just need a dancer :)

alexcjohnson
alexcjohnson reviewed Jun 17, 2021
View reviewed changes
src/components/drawing/index.js Outdated
@@ -500,7 +529,8 @@ drawing.pattern = function(sel, gd, patternID, shape, bgcolor, fgcolor, size, so
'id': fullID,
'width': width + 'px',
'height': height + 'px',
'patternUnits': 'userSpaceOnUse'
'patternUnits': 'userSpaceOnUse',
'patternTransform': isLegend ? 'scale(0.7)' : ''
Copy link
Collaborator

Choose a reason for hiding this comment

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

Can we adjust the size from the legend side, rather than detecting the legend inside here? Also do we need to scale down all these sizes? I'd rather we just restrict the maximum size to something we know will work, and perhaps when we have an array for pattern size just use the default size instead of the first one.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good calls.
See 5f5561f, a2a66e7 and 69f91e2.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Again though, do we need to scale down to 0.7 here? In the legend style we already limit the size to 12, can't we keep the same scale as on the graph, but if 12 is too big just reduce that limit?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Today in 38bd5ec I did try to avoid this scale reduction. But the problem is that the default size of 8 does not fit nicely in the legend icons and lose their identity.
So again in 178e0a6 I scaled down the patterns for icons using a greater ratio at 0.8.

Copy link
Member

Choose a reason for hiding this comment

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

I think the current legend items are "ok" ... better than on master, and we can always tweak them a bit more later. I really do need to get this merged and released so unless there's a hard blocker/pathological case here, can we defer the remaining work @alexcjohnson ?

Copy link
Collaborator

Choose a reason for hiding this comment

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

OK

alexcjohnson
alexcjohnson approved these changes Jun 18, 2021
View reviewed changes
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.

💃

@archmoj
Copy link
Contributor Author

archmoj commented Jun 18, 2021

The image test does not show a green light this branch at the moment.
While we updated flaky list on the master in #5737, let's see is we need to adjust it once again after the merge.
Merging...

@archmoj archmoj merged commit 16a14f5 into master Jun 18, 2021
@archmoj archmoj deleted the pattern-fgcolor-and-overlay-fillmode branch June 18, 2021 16:34
@archmoj archmoj mentioned this pull request Jun 18, 2021
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@nicolaskruchten nicolaskruchten nicolaskruchten left review comments

@alexcjohnson alexcjohnson alexcjohnson approved these changes

Assignees
No one assigned
Labels
status: reviewable type: bug type: feature
Projects
None yet
Milestone
v2.1.0
Development

Successfully merging this pull request may close these issues.

pattern bgcolor & fgcolor as well as overlay fillmode Missing colors in bar legend having a colorscale
3 participants
@archmoj @nicolaskruchten @alexcjohnson