Navigation Menu

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

Better x axis label formatting [WIP] #7103

Merged
merged 13 commits into from Mar 21, 2018
Merged

Better x axis label formatting [WIP] #7103

merged 13 commits into from Mar 21, 2018

Conversation

tlrobinson
Copy link
Contributor

@tlrobinson tlrobinson commented Mar 9, 2018

This PR resolves the following issues (which are all pretty much duplicates of each other):

TODO:

  • implement "compact" formatting for dates
  • implement 45° axis labels
  • implement 90° axis labels
  • implement logic to pick the best label formatting for ordinal axis
    • disable reducing tick granularity for ordinal axis
    • if overlap, rotate 45°, rotate 90°, or hide entirely, depending on space
    • don't rotate if not much vertical space
  • implement logic to pick best label formatting for numeric or timeseries axis
    • hide if overlapping
  • remove temporarily added setting for the x-axis formatting modes

Ideas:

  • initially render with all ticks, check for overlap, compute the max density that would prevent overlap?
    • difficult to do without basically reimplementing axis tick logic?
  • check if formatValue with { compact: true } would prevent overlap?
    • would require an additional render?
  • hard code different MIN_PIXELS_PER_TICK for different date granularities?
    • i.e. min pixels for day would be much larger than year since January 1, 2018 is much wider than 2018

@salsakran
Copy link
Contributor

Do days of the week come back as strings from the backend or are they mapped using moment?

@tlrobinson
Copy link
Contributor Author

@salsakran they come back as numbers and we use moment to format them

@mazameli
Copy link
Contributor

This is looking great. Looks like there are still some dashboard card scenarios we'll need to polish:

dash-cards

@tlrobinson
Copy link
Contributor Author

Thanks. Yeah that’s the “don't rotate if not much vertical space” TODO

Copy link
Contributor

@salsakran salsakran left a comment

Choose a reason for hiding this comment

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

Looks reasonable. Main question was whether to leave the settings enabled in case we get something off.

@@ -316,6 +316,16 @@ export const GRAPH_AXIS_SETTINGS = {
section: "Axes",
title: t`Show x-axis line and marks`,
widget: "toggle",
// widget: "select",
Copy link
Contributor

Choose a reason for hiding this comment

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

Why was this commented out? Thought we were going to leave it exposed.

chart.selectAll("g.x text").attr("transform", function() {
const { width, height } = this.getBBox();
return `translate(-${width /
2},${-height / 2}) rotate(${degrees}, ${width / 2}, ${height})`;
Copy link
Contributor

Choose a reason for hiding this comment

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

👍

/^rotate-(\d+)$/,
);
if (match) {
return -parseInt(match[1], 10);
Copy link
Contributor

Choose a reason for hiding this comment

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

just to state the obvious, this is because we want to read eg a 45 degree rotated label from lower left to upper right, rather than having it be read from upper left to lower right?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah it's rotating around the right edge of the label so we want to rotate it counter clockwise.

Actually i'm going to change this to return the un-negated rotation and negate it in onRenderRotateAxis.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'll add some comments too

const X_LABEL_MIN_SPACING = 2; // minimum space we want to leave between labels
const X_LABEL_ROTATE_90_THRESHOLD = 24; // tick width breakpoint for switching from 45° to 90°
const X_LABEL_HIDE_THRESHOLD = 12; // tick width breakpoint for hiding labels entirely
const X_LABEL_MAX_LABEL_HEIGHT_RATIO = 0.7; // percent rotated labels are allowed to take
Copy link
Contributor

Choose a reason for hiding this comment

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

My hunch is that 70% of the render area being taken up by labels is too high, but I'm down to wait until we see how this works on our internal instances before tweaking it further.

@tlrobinson tlrobinson merged commit 20846dd into master Mar 21, 2018
@tlrobinson tlrobinson deleted the feat-axis-labels branch March 21, 2018 23:29
@mazameli
Copy link
Contributor

Any reason we haven't closed out the issues that this PR resolves?

@salsakran
Copy link
Contributor

Only that I got used to issues being autoclosed by PR merges. You're welcome to do the honors if you don't want to wait for me =)

@lucasloami
Copy link

Hi guys, this feature is in v0.29.0-RC1? If yes, how can I test it? I'm using locally this version of Metabase (in docker) and I want to create a bar chart that has big strings in X axis (in a way they overlap) but I was not able to find where I can customize X axis labels' rotation.

Is there some "control" of rotation?

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.

None yet

4 participants