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

Horizontal legend traces wrap to new lines #786

Merged
merged 13 commits into from
Aug 2, 2016
Merged

Conversation

psalmody
Copy link
Contributor

Resolves #769

Iterate through traces, get maximum width, use margin and trace width to calculate if a new row is needed. Thanks @n-riesco for reviewing.

new horizontal legend images

@etpinard
Copy link
Contributor

@psalmody looking good. Thanks a lot for doing this.

To complete this PR:

  • Can you add an image test mock in the test/image/mocks/ directory showing off this new functionality? If you're feeling adventurous, you can try to generate the baseline yourself in our image test docker container (docs here).
  • Can you add a jasmine test case in test/jasmine/tests/legend_test.js making sure that changing the legend.orientation of a legend with many items makes it properly toggle between the scrollable and multi-column view

Let me know if you have any questions.

@etpinard etpinard added this to the v1.16.0 milestone Jul 28, 2016
@psalmody
Copy link
Contributor Author

psalmody commented Jul 28, 2016

@etpinard I added the mock and test. On the mock, I added more traces in the legend for better visibility (particularly in vertical legend orientation). I setup docker (I'm on Win10 and new to docker) and had some problems. Even extracting the linux commands from tasks/test_image.sh I'm having issues - appears that /var/www/streambed/image_server/plotly.js is empty in the docker container. Also - the readme makes it seem like this works with windows, but the tasks/test_image.sh file is not written for windows cmd.

@etpinard
Copy link
Contributor

@psalmody

Here's the link image baseline associated with the mock you added. Simpy download it and move it to test/image/baselines/legend_horizontal_autowrap.png.

Also - the readme makes it seem like this works with windows, but the tasks/test_image.sh file is not written for windows cmd.

Yeah that's correct. I've been wanting to rewrite the shell scripts found in tasks/ in node as opposed ever since the FOSS release. My apologies. I've opened up an issue (#794) this should be completed soon.


it('changed to vertical, scrollbar appeared', function(done) {
//changed orientation to vertical
expect(gd._fullLayout.legend.orientation).toBe('v');
Copy link
Contributor

@etpinard etpinard Jul 29, 2016

Choose a reason for hiding this comment

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

@psalmody can you rewrite this test as

Plotly.relayout(gd, 'legend.orientation', 'v').then(function() {
  /* assertions */

 return Plotly.relayout(gd, 'legend.orientation', 'h');
}.then(function() {
  /* assertions (back to base case) /*
});

and remove the assertion-less it('changing to vertical orientation', ... above? Thanks.

@etpinard
Copy link
Contributor

etpinard commented Aug 1, 2016

@psalmody Great.

I'll merge this PR in as soon as #786 (comment) is addressed. Thanks again for this PR!

@psalmody
Copy link
Contributor Author

psalmody commented Aug 2, 2016

@etpinard I updated the test (I think) the way you would like it. However, I had to completely destroy the graph and recreate in order to get the legend scrollbar to appear. Using Plotly.relayout does change the orientation but it puts the vertical legend overlapping the plot and doesn't show the scrollbar until you use the mouse wheel inside the area (at least on chrome). I'm guessing there are additional layout specifications that must be added by orientation vertical that would also need to be updated in Plotly.relayout in order to use that function.

@etpinard
Copy link
Contributor

etpinard commented Aug 2, 2016

.. recreate in order to get the legend scrollbar to appear

Oh. Looks like you discovered some buggy relayout behaviour in scrollable legends. Thanks for reporting 🍻 . I made an issue about it here: #808

Using Plotly.relayout does change the orientation but it puts the vertical legend overlapping the plot and doesn't show the scrollbar until you use the mouse wheel inside the area (at least on chrome).

Plotly.relayout should work here. But from what I understand, a lot of legend relayout cases (that have nothing to do with your PR) still need to be worked out.

So, I suggest removing the jasmine test you added in 3a2cf7b and patched in c31eb68 and instead add a jasmine test case testing some that relayout with some style attribute e.g. legend.bgcolor works as expected for mulit-line horizontal legends. Your patch in c31eb68 by calling Plotly.plot in sequence essentially makes the jasmine test the same code paths as the test image test.

@psalmody
Copy link
Contributor Author

psalmody commented Aug 2, 2016

Your patch in c31eb68 by calling Plotly.plot in sequence essentially makes the jasmine test the same code paths as the test image test.

Ah - that makes sense. Removed old tests and created new test as recommended.

@etpinard
Copy link
Contributor

etpinard commented Aug 2, 2016

@psalmody Great. Thanks very much for this PR. You get my vote for PR of the week 🏆

@etpinard etpinard merged commit 95c8353 into plotly:master Aug 2, 2016
@Braintelligence
Copy link

@psalmody Thank you for this PR! Do you have any idea why #3175 is happening, though? =/ I can't pinpoint where the code is at fault by my own 😿.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feature something new
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants