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

automatic height #337

Merged
merged 14 commits into from
May 3, 2021
Merged

automatic height #337

merged 14 commits into from
May 3, 2021

Conversation

Fil
Copy link
Contributor

@Fil Fil commented Apr 22, 2021

closes #323

It's not dramatically affecting any of the test plots (for example test/plots/athletes-sport-sex.js' height was fixed manually at 500px, and becomes 508px with this heuristic).

@Fil Fil requested a review from mbostock April 22, 2021 12:08
src/plot.js Outdated Show resolved Hide resolved
src/plot.js Outdated Show resolved Hide resolved
- y is always either ordinal or quantitative
- fy is always ordinal
@Fil Fil requested a review from mbostock May 1, 2021 17:11
src/plot.js Outdated Show resolved Hide resolved
@Fil
Copy link
Contributor Author

Fil commented May 2, 2021

I've revised all the test/plots/ :

  • mobyDickFaceted, penguinMassSex and penguinMassSexSpecies have become ultra-high (e.g. 770px vs 400)
  • morleyBoxPlot, movieProfitByGenre and ordinalBar have become short (160px, 320px and 200px) but I think it's ok
  • caltrain and ballotStatusRace are just a bit too tall (I think we should give them a manual height)

(I think that athletes-sport-weight is also a bit too tall—540 would be nicer than 640—, but that's already a manual setting.)

@Fil Fil force-pushed the fil/auto-height branch 2 times, most recently from 795fd23 to 86fece1 Compare May 2, 2021 08:25
@Fil Fil force-pushed the fil/auto-height branch from 86fece1 to 6a4065e Compare May 2, 2021 08:35
@Fil
Copy link
Contributor Author

Fil commented May 2, 2021

the large height comes from
42d95b4#diff-30ab29a77cf3887e9e7e3ec68157a17e77e891ca7d6335074ab995ab1f27b82fR141

my solution is to consider that as soon as a chart has vertical facets, the auto height of a facet should be halved:

-  const nfy = fy ? fy.scale.domain().length : 1;
-  return !!(y || fy) * Math.max(1, Math.min(60, ny * nfy)) * 20 + !!fx * 30 + 60;
+  const nfy = fy ? fy.scale.domain().length : 2;
+  return !!(y || fy) * Math.max(1, Math.min(60, ny * nfy)) * 10 + !!fx * 30 + 60;

In the test plots this change impacts only: ballotStatusRace, mobyDickFaceted, penguinMassSex and penguinMassSexSpecies.

@Fil Fil requested a review from mbostock May 2, 2021 09:01
@mbostock
Copy link
Member

mbostock commented May 3, 2021

I’ve committed a further tweak. I prefer that adding faceting does not affect the natural height of bands, but that if the subplots don’t have an ordinal y scale, that we partition the target height of 400px for each facet, but down to a minimum of ~140px for each facet.

@mbostock mbostock merged commit efe0030 into main May 3, 2021
@mbostock mbostock deleted the fil/auto-height branch May 3, 2021 16:25
@Fil
Copy link
Contributor Author

Fil commented May 3, 2021

(post-facto approved)

@Fil Fil mentioned this pull request Sep 3, 2021
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.

If y is ordinal, chose a smarter default height based on the domain’s cardinality?
2 participants