Fix embed themed models #5932
Fix embed themed models #5932
Conversation
|
AFAICT the only remaining place https://github.com/bokeh/bokeh/blob/master/bokeh/tests/test_model.py#L9-L37 I'd be +1 on removing it altogether in this PR. |
|
@bryevdv I can also probably get rid of |
examples/embed/embed_themed.py
Outdated
| p1 = figure(title='PINK') | ||
| p1.scatter(x1, y1) | ||
|
|
||
| theme = Theme(json={'attrs': {'Title': {'text_color': 'pink'}}}) |
There was a problem hiding this comment.
Maybe also add a dark (grey?) background, and make the CSS page background match? Then the lighter title will stand out more and also it gives an example of matching a plot to its background in the page.
There was a problem hiding this comment.
do you mean the background of the plot should match background-color for css class embed-wrapper?
My experience has been that if things like this aren't done immediately as the opportunity arises they often never get done, or languish. So I'm generally +1 on reasonable, incremental code gardening in "regular" PRs as opposed to giant "cleanup" PRs that try to do lots of it at once. |
|
After thinking about it some more it seems like maybe having a context manager for |
|
The example looks like this now: Pretty ugly, but proves the point I think. |
Maybe more like the crossdfilter demo? (in terms of colors) People ask for how to have a "dark" theme with some regularity so have a basic but reasonable one would be nice to demonstrate. |
bokeh/model.py
Outdated
| yield model | ||
| if len(existing_docs) == 0: | ||
| # no existing docs, use the current doc | ||
| doc = curdoc() |
There was a problem hiding this comment.
I'm not sure curdoc is advised here. Although I have not seen it done, it's not a huge stretch to imagine a Bokeh app that produces embedded stand alone bokeh plots as some kind of output. Then this code would intertwine the "produced" standalone plots with the bokeh app curdoc and I doubt that is desirable.
There was a problem hiding this comment.
that curdoc is what fixes the theme.
There was a problem hiding this comment.
It's one way, but it seems that copying any theme to a new Document should also be feasible?
There was a problem hiding this comment.
I think it fits the expectation that the curdoc would get rendered when embedding. This would happen for show() or save()
|
I'm not sure I favor keeping the context manager, but I guess I don't feel too strongly against it. But if we are going to do so we should own it, i.e. rename it |
examples/embed/embed_themed.py
Outdated
|
|
||
| p1 = figure(title='PINK TITLE') | ||
| p1.background_fill_color = "#999999" | ||
| p1.border_fill_color = "#999999" |
There was a problem hiding this comment.
these colors can go in the theme, sorry I was not explicit but I was suggesting making the theme have a bit more content.
|
|
|
This LGTM, you may need to merge master to get things green (see #5938) |
examples/embed/embed_themed.py
Outdated
| {{ script }} | ||
| <style> | ||
| body { | ||
| text-rendering: optimizeLegibility; |
There was a problem hiding this comment.
What's the purpose of setting this? text-rendering is a SVG property.
There was a problem hiding this comment.
I think I was copying from another example. I will take it out.
|
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. |
Changes to using curdoc rather than new doc when embedding
_ModelInEmptyDocumenthad been called