Skip to content

Closes #359: Truncate x-axis by default.#366

Merged
emtwo merged 1 commit intomasterfrom
emtwo/truncate
Apr 11, 2018
Merged

Closes #359: Truncate x-axis by default.#366
emtwo merged 1 commit intomasterfrom
emtwo/truncate

Conversation

@emtwo
Copy link
Copy Markdown

@emtwo emtwo commented Apr 11, 2018

No description provided.

@emtwo emtwo requested a review from washort April 11, 2018 19:27
scope.options.xAxisLabelLength = 300;
}
scope.$watch('options.globalSeriesType', (newType, oldType) => {
const defaultXAxisLength = 10;
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Why is this default set separately from DEFAULT_XAXIS_LABEL_LENGTH?

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

This default is used when someone is making a visualization for the first time whereas DEFAULT_XAXIS_LABEL_LENGTH is the fallback default when someone clears out the value of the label x-axis-label-length (found in chart-editor.html)

I think we can perhaps set DEFAULT_XAXIS_LABEL_LENGTH to 10 as well to be less confusing

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

OK sounds good. On further reflection I wonder if pie charts and line charts should have different defaults.

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

Yes, I would say there is probably a better/clearer way to set separate defaults for pie vs line charts. Though I realized that would be a bigger PR and need slightly more time (and we wanted to get this ready soon). For now this workaround does the trick to give pie charts a different default and I'm happy to file a follow-up.

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Great. r+

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.

2 participants