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

Add option to explicitly set chart legend position #4865

Merged
merged 4 commits into from
May 7, 2020
Merged

Conversation

kravets-levko
Copy link
Collaborator

@kravets-levko kravets-levko commented May 5, 2020

What type of PR is this? (check all applicable)

  • Feature

Description

Add a new option to control legend placement. Existing behavior (next to chart on width >= 600px and below when width < 600px) became "Auto", and added fixed position below the plot.

  • Add new option
  • Implement options
    • auto
    • below the plot

Related Tickets & Documents

#4852

Mobile & Desktop Screenshots/Recordings (if there are UI changes)

image

@kravets-levko
Copy link
Collaborator Author

@arikfr Please lmk if I'm on the right direction and feel free to share your ideas. Thank you!

@kravets-levko kravets-levko changed the base branch from visualizations-package to master May 6, 2020 08:21
@kravets-levko kravets-levko changed the base branch from master to visualizations-package May 6, 2020 08:21
@kravets-levko kravets-levko changed the base branch from visualizations-package to master May 6, 2020 08:31
Copy link
Member

@arikfr arikfr left a comment

Choose a reason for hiding this comment

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

We should have only two options:

  1. To the right of the chart with auto behavior like today. I don't see why the user would want it otherwise.
  2. To the bottom of the chart.

@kravets-levko kravets-levko marked this pull request as ready for review May 6, 2020 18:30
@kravets-levko
Copy link
Collaborator Author

I also experimented with other options, like placing legend above the plot, but that wasn't easy to implement (conflicted with modebar), so I abandoned this idea.

@arikfr
Copy link
Member

arikfr commented May 6, 2020

Is it easy to add an option of "inside the chart"? (regardless of whether it's overlapping or not)

Copy link
Member

@arikfr arikfr left a comment

Choose a reason for hiding this comment

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

Just a change to the option names to make them friendlier to users. Up to you if you want to change the naming in code.

@kravets-levko
Copy link
Collaborator Author

Is it easy to add an option of "inside the chart"? (regardless of whether it's overlapping or not)

Yes, it's possible. We can even choose anchors (all four corners). Would you like to see how it looks like?

Also, WDYT about moving "Show legend" checkbox to dropdown option ("Hide legend" or whatever) - to make UI less crowded?

@arikfr
Copy link
Member

arikfr commented May 6, 2020

Also, WDYT about moving "Show legend" checkbox to dropdown option ("Hide legend" or whatever) - to make UI less crowded?

I think it's a good idea.

Yes, it's possible. We can even choose anchors (all four corners). Would you like to see how it looks like?

If it's a small effort, then yes - let's do it. But maybe finish this one (with the "hide" in side the same dropdown) and try the new options in a separate PR? Then it will be easier to drop it if we don't like it.

@kravets-levko
Copy link
Collaborator Author

Okay, I'll move the checkbox to dropdown tomorrow morning (and add spelling fixes you suggested) and we'll finalize work on this PR. Then I'll prepare PR with more options.

@kravets-levko kravets-levko changed the title Add option to explicitly set chart legend position Add option to explicitly set chart legend position May 7, 2020
@kravets-levko kravets-levko merged commit dc49585 into master May 7, 2020
@kravets-levko kravets-levko deleted the fix-4852 branch May 7, 2020 14:21
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants