Skip to content

Axis names fix #601

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

Merged
merged 5 commits into from
Jul 20, 2018
Merged

Axis names fix #601

merged 5 commits into from
Jul 20, 2018

Conversation

dmt0
Copy link
Contributor

@dmt0 dmt0 commented Jul 19, 2018

addresses #575
closes #479

  • refactored axes component naming
  • correct axes name during creation
  • axes names in axes panel
  • add None option for Overlay

@dmt0 dmt0 self-assigned this Jul 19, 2018
@dmt0 dmt0 requested a review from nicolaskruchten July 19, 2018 17:54
@nicolaskruchten
Copy link
Contributor

@VeraZab can you review this as well please, given that you spent so much time working on this initially? :)

const multipleSublots =
fullLayout &&
fullLayout._subplots &&
Object.values(fullLayout._subplots).some(s => s.length > 1);
Copy link
Contributor

Choose a reason for hiding this comment

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

hmm, fullLayout._subplots is an object right
that contains subplotTypes as keys, and arrays as values, and we want to check that those arrays are of length greater than 1.

so isn't it more like:
Object.values(fullLayout._subplots).some(s => fullLayout[s].length > 1);

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Vera, it's Object.values(), not Object.keys()

Copy link
Contributor

Choose a reason for hiding this comment

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

okkk, yup, i see that now haha

@VeraZab
Copy link
Contributor

VeraZab commented Jul 19, 2018

other than the above, looks good to me!

@VeraZab
Copy link
Contributor

VeraZab commented Jul 19, 2018

💃

@nicolaskruchten
Copy link
Contributor

can you force-push here to re-trigger circle and percy plz?

@dmt0
Copy link
Contributor Author

dmt0 commented Jul 19, 2018

CircleCI is still down as of 6 minutes ago

@dmt0
Copy link
Contributor Author

dmt0 commented Jul 19, 2018

I'm thinking Overlay should have an explicit option for "none" - it took me a while to figure out that I have to clear it to spread out the plots.

@nicolaskruchten
Copy link
Contributor

Agreed, please add 'none'. This will close #479

@dmt0 dmt0 force-pushed the axis-names-fix branch from 4f5336f to df84b8b Compare July 19, 2018 20:43
@dmt0
Copy link
Contributor Author

dmt0 commented Jul 19, 2018

err, am I still passing review after all these commits? :)

@nicolaskruchten
Copy link
Contributor

@VeraZab please re-review if needed! I think the behaviour is fine for me. The only known-weird thing left outstanding with the secondary y-axis thing is plotly/plotly.js#2828

@dmt0 dmt0 force-pushed the axis-names-fix branch from df84b8b to c093fcb Compare July 19, 2018 21:38
@VeraZab
Copy link
Contributor

VeraZab commented Jul 20, 2018

ok rechecked looks good 💃

@dmt0 dmt0 merged commit 1919f50 into master Jul 20, 2018
@dmt0 dmt0 deleted the axis-names-fix branch July 20, 2018 15:29
@dmt0 dmt0 mentioned this pull request Aug 9, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

make the option to disable overlaying more discoverable
3 participants