pie : add Pattern #6601

thierryVergult · 2023-05-12T14:23:52Z

Patterns were already added to a few trace types in Plotly, like bar & scatter.

Hereby a first version to do so for pie charts, cf. Feature request: ability to add pattern fills to pie chart #6134

example

previous PRs : #5520 & #6101

Patterns were already added to a few trace types in Plotly, like bar & scatter. Hereby a first version to do so for pie charts, cf. Feature request: ability to add pattern fills to pie chart plotly#6134

only reference traceIn.marker.colors if traceIn.marker exists

src/traces/pie/defaults.js

archmoj · 2023-05-12T16:57:50Z

Another very exciting PR! 🏁
Please run npm run schema and commit the schema changes.
For your new mocks please add zz- to the start of their names i.e. to avoid encountering a bug we have on CircleCI.
Thank you!

Co-authored-by: Mojtaba Samimi <33888540+archmoj@users.noreply.github.com>

+ plot-schema.json after running npm run schema

thierryVergult · 2023-05-12T18:46:03Z

@archmoj : flaky-no-gl-jasmine failed, but I have no clue why, and I do not see the link with the code changes of this PR for the moment.

Then I will rework the StyleOne function.

archmoj · 2023-05-12T18:54:41Z

@archmoj : flaky-no-gl-jasmine failed, but I have no clue why, and I do not see the link with the code changes of this PR for the moment.

Then I will rework the StyleOne function.

flaky and webgl containers sometimes fail. I just rerun and please don't worry about it.

fixes bug (trace.marker.shape) & logic.

The styleOne function is also used by the funnelarea trace type, which has no pattern in it's schema at the moment, so extra check if pattern exists, before checking if a shape exists within the pattern.

thierryVergult · 2023-05-13T18:58:51Z

@archmoj : any idea how to transform / avoid the code below (from styleOne) into a more readable version? When styleOne is called for the legend, the i attribute is sitting a level deeper (probably since each slice of the pie is becoming a legend item)), so I brought it one level up, so it gets recognized in the Drawing.pointStyle function.

if(s[0][0].__data__.i === undefined) {
        // coming from a legend
        s[0][0].__data__.i = s[0][0].__data__[0].i;
    }

funnelarea pixel tests were broken, so use the point color in way more cases

thierryVergult · 2023-05-14T08:59:01Z

@archmoj : Is there a way to run the mock tests (find visual differences) locally?

add 5 pie charts, spread over the x domain, with increasing pattern functionalities

archmoj · 2023-05-16T17:58:23Z

Please replace the baseline with this file.

archmoj · 2023-05-16T17:59:47Z

@archmoj : Is there a way to run the mock tests (find visual differences) locally?

Yes. Please see here.

thierryVergult · 2023-05-17T09:32:07Z

Please replace the baseline with this file.

Getting an error

<Error>
<Code>AccessDenied</Code>
<Message>Request has expired</Message>
<X-Amz-Expires>60</X-Amz-Expires>

archmoj · 2023-05-17T13:46:59Z

Please find the baseline in the Artifacts tab here: https://app.circleci.com/pipelines/github/plotly/plotly.js/8647/workflows/b943aecb-e8c5-4ad3-9615-f0b994869e9f/jobs/190922/artifacts

- take the index from the point in the styleOne function, instead of setting it before in the data structure - zz-pie_pattern : height 500

archmoj · 2023-05-17T14:43:49Z

Would you please investigate why pie_style_arrays mock renders differently?
You will find the diff here: https://app.circleci.com/pipelines/github/plotly/plotly.js/8665/workflows/be5e9dfa-6855-4109-8a70-9f13a88aeab1/jobs/191244/artifacts

thierryVergult · 2023-05-17T16:12:43Z

I reverted a change to fix it. Now a timezone test fails, on an Ubuntu file. No clue

…

On Wed, 17 May 2023, 16:44 Mojtaba Samimi, ***@***.***> wrote: Would you please investigate why pie_style_arrays mock renders differently? You will find the diff here: https://app.circleci.com/pipelines/github/plotly/plotly.js/8665/workflows/be5e9dfa-6855-4109-8a70-9f13a88aeab1/jobs/191244/artifacts — Reply to this email directly, view it on GitHub <#6601 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/ACYGOP26L7WINVIE5FBPGDLXGTP3BANCNFSM6AAAAAAX7TJVTE> . You are receiving this because you authored the thread.Message ID: ***@***.***>

archmoj · 2023-05-17T16:52:17Z

I reverted a change to fix it. Now a timezone test fails, on an Ubuntu file. No clue
…
On Wed, 17 May 2023, 16:44 Mojtaba Samimi, @.> wrote: Would you please investigate why pie_style_arrays mock renders differently? You will find the diff here: https://app.circleci.com/pipelines/github/plotly/plotly.js/8665/workflows/be5e9dfa-6855-4109-8a70-9f13a88aeab1/jobs/191244/artifacts — Reply to this email directly, view it on GitHub <#6601 (comment)>, or unsubscribe https://github.com/notifications/unsubscribe-auth/ACYGOP26L7WINVIE5FBPGDLXGTP3BANCNFSM6AAAAAAX7TJVTE . You are receiving this because you authored the thread.Message ID: @.>

No worries. That sometimes happens.
Everything passed on CircleCI when I rerun.

alexcjohnson · 2023-05-24T18:49:19Z

test/image/mocks/zz-pie_pattern.json

+          "bgcolor": "lightgrey"
+        }
+      },
+      "textfont": { "color": ["black", "white", "black"]},


I wonder if we can get this to be the default result for inside text? This would particularly help in the cases where we don't know whether the text will end up inside or outside. Maybe if the pattern solidity <0.5 we contrast with the bgcolor, >0.5 we contrast with the fgcolor? Here's where this auto color is chosen (Color.contrast picks either black or white, whichever is farther from the given color):

plotly.js/src/traces/pie/plot.js

Line 531 in 0919988

color: customColor || Color.contrast(pt.color),

Note I haven't looked but wonder if we want to do the same for bars:

plotly.js/src/traces/bar/style.js

Line 124 in 0919988

color: Color.contrast(barColor),

I checked similar cases for bar trace and scatter trace, see attached. Scatter trace with area filled with a pattern seems a special case anyway, but also the actual bar trace is deciding on the color of the text inside the bar by evaluating the base color of the bar, without taking into account pattern attributes.

And even flipping the text color to black inside the bar / slice will give a distortion effect, probably as bad as the actual situation. Patterns with text inside is probably never a very good idea ..

I would prefer to add a pattern to a few other trace types first (funnelarea, sunburst, ..) and to handle the color of the text inside a pattern eventually later via a separate PR.

zz-pattern_textfont_color.json.txt

Hmm OK - I guess it's not so clear that any color we choose here will actually be usable, for a lot of situations neither black nor white would really work and (given that this is often used in black&white contexts) we don't want to consider any other colors. So the recommendation I guess would be to use textposition='outside' when you have patterns.

alexcjohnson

💃 LGTM!

thierryVergult added 2 commits May 12, 2023 16:17

pie : add Pattern

342bd09

Patterns were already added to a few trace types in Plotly, like bar & scatter. Hereby a first version to do so for pie charts, cf. Feature request: ability to add pattern fills to pie chart plotly#6134

piePattern : add md & fix reference to colors

1cce254

only reference traceIn.marker.colors if traceIn.marker exists

archmoj added feature something new community community contribution status: reviewable labels May 12, 2023

archmoj reviewed May 12, 2023

View reviewed changes

src/traces/pie/defaults.js Outdated Show resolved Hide resolved

archmoj added the status: has TODOs label May 12, 2023

thierryVergult and others added 2 commits May 12, 2023 19:28

Update src/traces/pie/defaults.js

fedbe67

Co-authored-by: Mojtaba Samimi <33888540+archmoj@users.noreply.github.com>

piePattern : use the old point color when no shape present

c24ae98

+ plot-schema.json after running npm run schema

thierryVergult added 2 commits May 13, 2023 15:16

piePattern : rework styleOne in case no colors defined

e40e078

fixes bug (trace.marker.shape) & logic.

piePattern : extra check on trace.marker.pattern

a683c2a

The styleOne function is also used by the funnelarea trace type, which has no pattern in it's schema at the moment, so extra check if pattern exists, before checking if a shape exists within the pattern.

piePattern : fix broken tests

49319de

funnelarea pixel tests were broken, so use the point color in way more cases

piePattern : add mock

8cd2c97

add 5 pie charts, spread over the x domain, with increasing pattern functionalities

thierryVergult added 2 commits May 17, 2023 16:07

piePattern : rework the index of legend items

5d809b6

- take the index from the point in the styleOne function, instead of setting it before in the data structure - zz-pie_pattern : height 500

piePattern : lint fix & d3 call style

a422094

piePattern : call pointStyle outside the d3 s.style construction

f376b63

archmoj removed the status: has TODOs label May 17, 2023

archmoj requested a review from alexcjohnson May 23, 2023 13:58

alexcjohnson reviewed May 24, 2023

View reviewed changes

alexcjohnson approved these changes May 25, 2023

View reviewed changes

archmoj merged commit 23fc32b into plotly:master May 30, 2023

archmoj mentioned this pull request May 31, 2023

add pattern to funnelarea #6619

Merged

thierryVergult deleted the piePattern branch June 1, 2023 09:16

thierryVergult mentioned this pull request Jun 1, 2023

add pattern to sunburst #6622

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

pie : add Pattern #6601

pie : add Pattern #6601

thierryVergult commented May 12, 2023 •

edited

Loading

archmoj commented May 12, 2023

thierryVergult commented May 12, 2023

archmoj commented May 12, 2023

thierryVergult commented May 13, 2023 •

edited

Loading

thierryVergult commented May 14, 2023

archmoj commented May 16, 2023

archmoj commented May 16, 2023

thierryVergult commented May 17, 2023

archmoj commented May 17, 2023

archmoj commented May 17, 2023

thierryVergult commented May 17, 2023 via email

archmoj commented May 17, 2023

alexcjohnson May 24, 2023

thierryVergult May 25, 2023

alexcjohnson May 25, 2023

alexcjohnson left a comment

pie : add Pattern #6601

pie : add Pattern #6601

Conversation

thierryVergult commented May 12, 2023 • edited Loading

archmoj commented May 12, 2023

thierryVergult commented May 12, 2023

archmoj commented May 12, 2023

thierryVergult commented May 13, 2023 • edited Loading

thierryVergult commented May 14, 2023

archmoj commented May 16, 2023

archmoj commented May 16, 2023

thierryVergult commented May 17, 2023

archmoj commented May 17, 2023

archmoj commented May 17, 2023

thierryVergult commented May 17, 2023 via email

archmoj commented May 17, 2023

alexcjohnson May 24, 2023

Choose a reason for hiding this comment

thierryVergult May 25, 2023

Choose a reason for hiding this comment

alexcjohnson May 25, 2023

Choose a reason for hiding this comment

alexcjohnson left a comment

Choose a reason for hiding this comment

thierryVergult commented May 12, 2023 •

edited

Loading

thierryVergult commented May 13, 2023 •

edited

Loading