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

Avoid tight daily labels on axes with rangebreaks when using "day of week" pattern #4695

Merged
merged 7 commits into from
Mar 27, 2020

Conversation

archmoj
Copy link
Contributor

@archmoj archmoj commented Mar 27, 2020

Demo

Follow up of #4614 and #4688:
Commit c340efc exercised 2 -> 7 rounding which leads to following commits.

As a result we only round dtick to 1 if it is really close to 1.

@plotly/plotly_js

@archmoj archmoj added bug something broken status: reviewable labels Mar 27, 2020
@nicolaskruchten
Copy link
Contributor

Hmm yeah that mock does seem sparse. But I think erring on the side of sparseness is better than sometimes having unreadably-dense plots like this one:

image

@archmoj
Copy link
Contributor Author

archmoj commented Mar 27, 2020

Hmm yeah that mock does seem sparse. But I think erring on the side of sparseness is better than sometimes having unreadably-dense plots like this one:

image

@nicolaskruchten thanks very much for posting the image related to the ( old ) undesirable behaviour.

if(ax.dtick === 2 * ONEDAY) ax.dtick = ONEDAY;
else if(ax.dtick === 3 * ONEDAY) ax.dtick = 7 * ONEDAY;
ax.dtick = roundDTick(roughDTick, ONEDAY, [1, 1.001, 7, 14]);
if(ax.dtick !== ONEDAY) ax.dtick = 7 * ONEDAY;
Copy link
Collaborator

Choose a reason for hiding this comment

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

Is this different from just saying roundDTick(roughDTick, ONEDAY, [1, 7, 14])? The underlying function is Lib.roundUp so I'd expect anything too big for 1 would already bump up to 7. Also I think this would convert 2-week ticks into 1-week?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Also I think this would convert 2-week ticks into 1-week?

Doog eye. Fixed in 79b6d95.

But we still need to fix something possibly in Lib.dateTick0 to compute dtick on those reversed cases.

Copy link
Collaborator

Choose a reason for hiding this comment

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

again though, is this different from just roundDTick(roughDTick, ONEDAY, [1, 7, 14])?

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 think that is different. The issue was almost related to numbers close to 1 (e.g. 2) not those close to 7. Anyway it seems there is no more need for that hacy roundingSet as the logic is changed in the commits below.

@archmoj
Copy link
Contributor Author

archmoj commented Mar 27, 2020

@nicolaskruchten would you mind testing the updated demo in the PR description.
Thanks!

Copy link
Collaborator

@alexcjohnson alexcjohnson left a comment

Choose a reason for hiding this comment

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

💃 sometimes the simple solution is the best 😅

@archmoj archmoj merged commit 10e7fd0 into master Mar 27, 2020
@archmoj archmoj deleted the rangebreaks-avoid-tight-day-of-week-labels branch March 27, 2020 17:14
@archmoj archmoj added this to the v1.53.0 milestone Mar 27, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug something broken
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants