Skip to content

automatic height - #337

Merged
mbostock merged 14 commits into
mainfrom
fil/auto-height
May 3, 2021
Merged

automatic height#337
mbostock merged 14 commits into
mainfrom
fil/auto-height

Conversation

@Fil

@FilFil commented Apr 22, 2021

Copy link
Copy Markdown
Contributor

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 mbostockApril 22, 2021 12:08
Comment threadsrc/plot.js Outdated
Comment threadsrc/plot.js Outdated
- y is always either ordinal or quantitative
- fy is always ordinal
@Fil
Fil requested a review from mbostockMay 1, 2021 17:11
Comment threadsrc/plot.js Outdated
@Fil

Fil commented May 2, 2021

Copy link
Copy Markdown
ContributorAuthor

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
Filforce-pushed the fil/auto-height branch 2 times, most recently from 795fd23 to 86fece1CompareMay 2, 2021 08:25
@Fil
Filforce-pushed the fil/auto-height branch from 86fece1 to 6a4065eCompareMay 2, 2021 08:35
@Fil

Fil commented May 2, 2021

Copy link
Copy Markdown
ContributorAuthor

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 mbostockMay 2, 2021 09:01
@mbostock

Copy link
Copy Markdown
Member

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 mainMay 3, 2021
@mbostock
mbostock deleted the fil/auto-height branch May 3, 2021 16:25
@Fil

Fil commented May 3, 2021

Copy link
Copy Markdown
ContributorAuthor

(post-facto approved)

@FilFil mentioned this pull request Sep 3, 2021
Sign up for freeto 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

@Fil@mbostock