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 chart.responsiveResize(true) option to switch to viewBox resizing… #1312

Closed
wants to merge 2 commits into from
Closed

Add chart.responsiveResize(true) option to switch to viewBox resizing… #1312

wants to merge 2 commits into from

Conversation

atomless
Copy link

… instead of fixed width and height attributes

… instead of fixed width and height attributes
@gordonwoodhull
Copy link
Contributor

The implementation looks nice and clean. I'll need at least two tests

  1. asserting that width and height are not set, and viewBox is set, when this flag is set
  2. the contrapositive

As I've noted, this is only one of many strategies for responsive resizing, so I'd prefer the name to have viewBox in in it somewhere.

@gordonwoodhull
Copy link
Contributor

Thanks @atomless!

@atomless
Copy link
Author

atomless commented Jun 15, 2017

@gordonwoodhull, good point. I'll change the option name to .useViewBox(bool)

Can you offer any pointers on the tests, I normally use Ava for tests so am not familia with this style. I presume it should go in the base-mixin-spec?

@gordonwoodhull
Copy link
Contributor

Yes, it should go in the base-mixin-spec.

I know writing tests in an alien style is a burden; just do your best on that part. Checking whether an attr exists would be something like

   expect(chart.svg().attr('viewBox')).not.toBeNull();

Also, for future reference, it's best not to check in the artifacts when submitting a PR. This is easy for me to clean up, though.

@atomless
Copy link
Author

atomless commented Jun 15, 2017

Okay. More specifically, in the base-mixin-spec, I can't locate a test for the width and height attr that normally gets added to the root svg and that the viewBox attr replaces if enabled. Seems the viewbox test should belong near that test or is there not one for that?

@gordonwoodhull
Copy link
Contributor

gordonwoodhull commented Jun 15, 2017

Yup, looks like we don't check that attribute in particular. (We test .width() and .height() for a lot of charts, but getting total coverage for a library with this scope would be really difficult.)

Thematically, I'd guess this belongs after the "calculation of dimensions" test, since it's another method for responsive layout.

@gordonwoodhull
Copy link
Contributor

Hi @atomless, I'm in the process of merging this for dc.js 2.1.7.

I'm updating the examples in http://dc-js.github.io/dc.js/resizing/ so that we can test them by eye - please check it out once this is released. Just click on any of the examples and then add resize=viewbox.

Currently it applies .width(600).height(400) which seems to work pretty well for the text sizes. Is that about what you use?

I haven't implemented the heatmap example because the screen is divided. Could do it with a little work.

We can continue to refine this feature in 2.1.* as we get more experience with it.

gordonwoodhull added a commit that referenced this pull request Jun 23, 2017
gordonwoodhull added a commit that referenced this pull request Jun 23, 2017
gordonwoodhull added a commit that referenced this pull request Jun 23, 2017
gordonwoodhull added a commit that referenced this pull request Jun 23, 2017
@gordonwoodhull
Copy link
Contributor

Released in 2.1.7 under the name useViewBoxResizing. Test and example commits are linked above.

It works pretty well. Thanks @atomless!

@dahlbyk
Copy link
Contributor

dahlbyk commented May 7, 2019

SEO bump: try using this if you need a dc.js print stylesheet.

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.

3 participants