-
-
Notifications
You must be signed in to change notification settings - Fork 4.2k
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
Add CustomJSTicker #13988
Add CustomJSTicker #13988
Conversation
8afbd36
to
6d1c67c
Compare
One thing that occurred to me after making the example: I suspect many of the previous questions about limiting "too many" categorical labels would probably be fine with keeping the ticks, and only thinning the labels. However, these two things are currently implicitly linked. In the example I did save the original ticker on the grid, so that all the grid lines do show up. This seems like it would be a reasonable compromise for most cases IMO, but we can consider splitting off a separate optional |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've found two typos and some missing whitespaces. Nothing really important
Co-authored-by: Moritz Schreiber <[email protected]>
@mattpap are you done commenting on this PR (and the other one)? |
@mattpap I would like to merge this, do you have any comments? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good. However, I would mark the new model as experimental, to give ourselves time to resolve #13994, without having to go through unnecessary deprecations.
It would strike me as an extremely odd incongruity to mark an API that is identical to all other existing "CustomJS" models as experimental. We are going to have to go through them all in any case, so I don't see the value in manufacturing a special case. FWIW my hope for #13994 was to consolidate implementation details, and possibly extend some functionality that only exists in |
* Add CustomJSTicker * Apply suggestions from code review Co-authored-by: Moritz Schreiber <[email protected]> --------- Co-authored-by: Moritz Schreiber <[email protected]>
This pull request has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs. |
@mattpap we seem to have a fair amount of both discrepancy and duplication in the handling of the various CustomJS models at this point. After this PR and the other PR are merged I'd like to make an issue to hammer out a refactoring plan that streamlines all the CustomJS models and makes them as consistent as possible.