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

griddash #6144

Merged
merged 29 commits into from
Mar 23, 2022
Merged

griddash #6144

merged 29 commits into from
Mar 23, 2022

Conversation

njwhite
Copy link
Contributor

@njwhite njwhite commented Mar 17, 2022

Resolves #4530 and resolves #4045 : add a griddash property that follows the pattern of gridwidth and gridcolor.

@njwhite njwhite changed the title first go at feature griddash Mar 17, 2022
@archmoj archmoj added feature something new community community contribution labels Mar 18, 2022
@njwhite
Copy link
Contributor Author

njwhite commented Mar 19, 2022

Hi @archmoj , thanks for the review. I've had a look at why the CI is failing. The 3 baseline diffs are all around text rendering on plots with no gridlines at all - do you know how my PR could be affecting these? Are these tests flakey? Thanks -

@archmoj
Copy link
Contributor

archmoj commented Mar 20, 2022

Hi @archmoj , thanks for the review. I've had a look at why the CI is failing. The 3 baseline diffs are all around text rendering on plots with no gridlines at all - do you know how my PR could be affecting these? Are these tests flakey? Thanks -

The tests are flaky. To avoid that please rename your new mocks to start with 'z-' so they don't affect the current rendering order.
Thanks.

@njwhite
Copy link
Contributor Author

njwhite commented Mar 20, 2022

@archmoj thanks, the baseline tests now pass!

draftlogs/6144_add.md Outdated Show resolved Hide resolved
draftlogs/6144_add.md Outdated Show resolved Hide resolved
@archmoj
Copy link
Contributor

archmoj commented Mar 23, 2022

@njwhite Thanks very much for the PR 🏆
It looks like implementing this feature for gl3d subplot requires additional work. So let's skip that in this PR by reverting the changes made to src/plots/gl3d/*.
Then please update the schema to reflect all the changes.
For the image tests, there is a mock named plot_types containing some subplots. Let's try adding a similar one by excluding the gl3d and the pie then add a polar and a smith graph and also add (or modify) a second mock for carpet showing minorgriddash and giddash options please 🙏

@njwhite
Copy link
Contributor Author

njwhite commented Mar 23, 2022

Thanks @archmoj - I've done everything on your list. Some notes:

  • The zz00- prefixing looks a bit messy, it might be useful to renumber all the tests in a different PR to fix the ordering
  • The tests started failing on this dev branch when master was merged in here. The error is unhelpful (below) and doesn't seem related to the code changes so might be a build infrastructure issue?
 Uncaught Error: Cannot find module '/home/circleci/plotly.js/test/jasmine/tests/animate_test.js'
  Error: Cannot find module '/home/circleci/plotly.js/test/jasmine/tests/animate_test.js'
      at o (/tmp/node_modules/browser-pack/_prelude.js:1:1 <- /tmp/19e1c3e4a7c1296c8a59b89b2cf5cc84.browserify.js:1:167)
      at o (/tmp/node_modules/browser-pack/_prelude.js:1:1 <- /tmp/19e1c3e4a7c1296c8a59b89b2cf5cc84.browserify.js:1:133)
      at test/jasmine/tests/animate_test.js:1:34

Thanks -

@archmoj
Copy link
Contributor

archmoj commented Mar 23, 2022

  • The tests started failing on this dev branch when master was merged in here. The error is unhelpful (below) and doesn't seem related to the code changes so might be a build infrastructure issue?
 Uncaught Error: Cannot find module '/home/circleci/plotly.js/test/jasmine/tests/animate_test.js'
  Error: Cannot find module '/home/circleci/plotly.js/test/jasmine/tests/animate_test.js'
      at o (/tmp/node_modules/browser-pack/_prelude.js:1:1 <- /tmp/19e1c3e4a7c1296c8a59b89b2cf5cc84.browserify.js:1:167)
      at o (/tmp/node_modules/browser-pack/_prelude.js:1:1 <- /tmp/19e1c3e4a7c1296c8a59b89b2cf5cc84.browserify.js:1:133)
      at test/jasmine/tests/animate_test.js:1:34

There is a memory issue. Please no worry about that one as I could simply rerun the test.

@njwhite
Copy link
Contributor Author

njwhite commented Mar 23, 2022

@archmoj great, I've removed / renamed the mocks -

src/plots/gl2d/convert.js Outdated Show resolved Hide resolved
draftlogs/6144_add.md Outdated Show resolved Hide resolved
@njwhite
Copy link
Contributor Author

njwhite commented Mar 23, 2022

@archmoj ok no more gl2d

@archmoj
Copy link
Contributor

archmoj commented Mar 23, 2022

💃 💃 💃
💃 💃 💃
💃 💃 💃

@archmoj archmoj merged commit 8db1646 into plotly:master Mar 23, 2022
kMutagene added a commit to plotly/Plotly.NET that referenced this pull request Feb 3, 2023
…th, ternary and geo subplots and add griddash and minorgriddash to carpet trace (plotly/plotly.js#6144), adapt StyleParam.DrawingStyle accordingly
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
community community contribution feature something new
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Customizing style (dashed, dotted) of grid lines New layout[axisid].griddash attribute
3 participants