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

Add CustomJSTicker #13988

Merged
merged 2 commits into from
Jul 31, 2024
Merged

Add CustomJSTicker #13988

merged 2 commits into from
Jul 31, 2024

Conversation

bryevdv
Copy link
Member

@bryevdv bryevdv commented Jul 20, 2024

@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.

@bryevdv bryevdv requested a review from mosc9575 July 20, 2024 04:34
@bryevdv
Copy link
Member Author

bryevdv commented Jul 20, 2024

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 labels_ticker in the future if it really seems needed.

Copy link
Contributor

@mosc9575 mosc9575 left a 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

src/bokeh/models/tickers.py Outdated Show resolved Hide resolved
src/bokeh/models/tickers.py Outdated Show resolved Hide resolved
examples/styling/plots/custom_js_ticker.py Outdated Show resolved Hide resolved
Co-authored-by: Moritz Schreiber <[email protected]>
@bryevdv
Copy link
Member Author

bryevdv commented Jul 22, 2024

@mattpap are you done commenting on this PR (and the other one)?

@bryevdv
Copy link
Member Author

bryevdv commented Jul 30, 2024

@mattpap I would like to merge this, do you have any comments?

Copy link
Contributor

@mattpap mattpap left a 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.

@mattpap mattpap added this to the 3.6 milestone Jul 31, 2024
@bryevdv
Copy link
Member Author

bryevdv commented Jul 31, 2024

I would mark the new model as experimental

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 CustomJS to other models, i.e. additive and/or hidden changes.

@bryevdv bryevdv merged commit 54252f8 into branch-3.6 Jul 31, 2024
25 checks passed
@bryevdv bryevdv deleted the bv/customjs-ticker branch July 31, 2024 16:07
Chiemezuo pushed a commit to Chiemezuo/bokeh that referenced this pull request Aug 27, 2024
* Add CustomJSTicker

* Apply suggestions from code review

Co-authored-by: Moritz Schreiber <[email protected]>

---------

Co-authored-by: Moritz Schreiber <[email protected]>
Copy link

github-actions bot commented Nov 8, 2024

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.

@github-actions github-actions bot locked as resolved and limited conversation to collaborators Nov 8, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add CustomJSTicker
3 participants