-
-
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 support for canvas and DOM rendering to Legend
#14028
Conversation
@mattpap should the linked issues be triaged as |
Yes. I'm actually triaging issues that may be affected by this work.
Axes and title will have to be next. Then it will be possible to work on color bars. |
102d16f
to
97d3708
Compare
I was hoping to finish this PR as a big feature for bokeh 3.6, but that's unrealistic given how much additional work needed to be done to support DOM rendering and how much more is needed to tweak and fine tune the implementation. What needs to be done is:
|
e683c93
to
63dec74
Compare
7700aa7
to
5cf9c13
Compare
c993e03
to
150f145
Compare
150f145
to
f805003
Compare
Legend
There is still one regression to address, but otherwise this is ready for review. Note that great many changes to the internals are more or less temporary, and will get refined in future PRs touching the subject. These will include changes to axes, labels, title and text glyphs. |
7448ed1
to
1db2433
Compare
@mattpap one of the images at least seems to have an issue? |
It's fixed now. |
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 didn't have a chance to go over the the JS super carefully, but I did not see any obvious issues. I did go over the images quite carefully. @mattpap if you'd like more code review I'd encourage you to request from @philippjfr or @ianthomas23 directly. Otherwise, probably just better to get this merged earlier to see how it shakes out.
Test failure is unrelated and reported in issue #14128. I'm going ahead and merge this. There is still a lot of temporary stuff in this PR or stuff that require generalization, that when refined in hopefully in smaller PRs, will be easier for reviewers to digest. |
I see that this has been merged and I have only given it a cursory look so far. I was asked to check this work in Safari specifically. So I built the js and the docs with this PR applied and the first plot that I opened (transform markers) I noticed a pretty big difference between Chrome versus Safari. Chrome: Safari: |
@gabalafou, I can reproduce this in epiphany (webkit on Linux). Can you start an issue for this? Although I can reproduce the issue, that browser is insufferable and I can't inspect or debug anything in it at this point at least. |
I've started an issue here: #14134 I added the |
Thanks.
That's a good start. |
The goal of this PR is allow both canvas and DOM/CSS rendering for renderers for which it makes sense, i.e. renderers that are UI components and renderers that include text. Secondary goal is to replace custom layouts with CSS layout where every possible without a major redesign. I expect only a selected subset of renderers will be covered in this PR. I started with
Legend
because it touches every aspect that I need to cover by the new design.The effects of this PR will be:
Screencast.from.16.08.2024.11.49.27.webm