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

Added better support for computing bokeh plot size #1140

Merged
merged 1 commit into from Feb 21, 2017

Conversation

Projects
None yet
2 participants
@philippjfr
Member

philippjfr commented Feb 20, 2017

The current BokehRenderer.get_size method only supports very simple plots that are not part of a layout grid, or contain toolbars and tables. This PR computes the size of a bokeh plot more accurately for all these cases, which can be very useful when wanting to embed the plot in HTML and know the size ahead of time.

@philippjfr philippjfr added the bokeh label Feb 20, 2017

@philippjfr philippjfr referenced this pull request Feb 20, 2017

Merged

Added View parameters and widgets #27

2 of 2 tasks complete
@philippjfr

This comment has been minimized.

Member

philippjfr commented Feb 20, 2017

Ready for review and merge.

width, height = plot.plot_width, plot.plot_height
elif isinstance(plot, (Plot, Div, DataTable)):
width, height = plot.width, plot.height
if width is None: width = 0

This comment has been minimized.

@jlstevens

jlstevens Feb 20, 2017

Member

Isn't this an error condition? What are you going to do with a plot that is either zero width, zero height or both?

This comment has been minimized.

@philippjfr

philippjfr Feb 20, 2017

Member

Certain models do not declare a width/height and rather than guess at it I'd take the hit and just say it's zero. The only thing that I've found that's actually affected by this is the Div model used to add titles to grid plots.

This comment has been minimized.

@philippjfr

philippjfr Feb 20, 2017

Member

Perhaps I'll just add an explicit branch for the Div model, and make a comment about this.

This comment has been minimized.

@jlstevens

jlstevens Feb 20, 2017

Member

Perhaps I'll just add an explicit branch for the Div model, and make a comment about this.

Sounds like a good idea.

@jlstevens

This comment has been minimized.

Member

jlstevens commented Feb 20, 2017

I made one comment (although I can't say I follow all the detailed size computation logic!) but generally it looks good.

The main thing I checked was that get_size is defined as a @bothmethod which means it is easy to get the size of bokeh plot as long as you can import the BokehRenderer class.

Happy to merge once you address my comment.

@philippjfr

This comment has been minimized.

Member

philippjfr commented Feb 21, 2017

@jlstevens Ready to merge.

@jlstevens

This comment has been minimized.

Member

jlstevens commented Feb 21, 2017

Looks good. Merging.

@jlstevens jlstevens merged commit 938ee7c into master Feb 21, 2017

4 checks passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
continuous-integration/travis-ci/push The Travis CI build passed
Details
coverage/coveralls Coverage increased (+0.03%) to 78.197%
Details
s3-reference-data-cache Test data is cached.
Details

@jbednar jbednar deleted the bokeh_renderer_get_size branch Feb 22, 2017

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment