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

Table #8303

Closed
wants to merge 8 commits into from
Closed

Table #8303

wants to merge 8 commits into from

Conversation

swfiua
Copy link
Contributor

@swfiua swfiua commented Mar 15, 2017

Allow edge colours for the cells making up the table to be supplied.

Using a lighter edge colour makes the numbers easier to read.

Fix auto_set_font_size to check height of text as well as width.

Use "center_baseline" as the vertical text alignment as it seems to centre better within cells.

also make auto selecting font size pay attention to height as
well as width
Copy link
Member

@phobson phobson left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could you add a test demonstrating the new functionality?

@@ -1192,7 +1192,7 @@ def set_verticalalignment(self, align):

ACCEPTS: [ 'center' | 'top' | 'bottom' | 'baseline' ]
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

should we add 'center_baseline' here too?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yes -- will do.

@phobson phobson added this to the 2.1 (next point release) milestone Mar 15, 2017
now enlarges the font as well as shrinking to fit the
available space
@swfiua
Copy link
Contributor Author

swfiua commented Mar 15, 2017

I just took a look at the table tests. The image based tests will need updating anyway.

The images will have changed subtly in some cases.

@story645
Copy link
Member

can you please post some images with the new functionality?

@swfiua
Copy link
Contributor Author

swfiua commented Mar 15, 2017

There will be some new images with the new tests -- once I have written them ;)

@swfiua
Copy link
Contributor Author

swfiua commented Mar 15, 2017

For now, examples/pylab_examples/table_demo.py uses the new cellEdgeColour option.

Fix special case where required dimensions are zero.

This happens if cell has a colour but no text.

Update baseline_images for table tests.

Changes are all due to small changes in text placement and or font
size.
@swfiua
Copy link
Contributor Author

swfiua commented Mar 15, 2017

I've added a test for the cellEdgeColour(s) options.

This commit 57c1aef

Has an example (in the baseline_images for tests) that shows the use of edge colours.

The other part of this fix is to make the "auto_set_font_size" code a bit smarter and the use of 'center_baseline' to align the text vertically.

Overall, the tables are more pleasing to the eye.

I fixed the examples/pylab_examples/table_demo.py to use a light grey for the cell edge colours -- this makes it easier to pick out the cell text.

@NelleV
Copy link
Member

NelleV commented Mar 15, 2017

I didn't have time to do a full review, but can you also make sure to not expose new functions to Matplotlib's API unless absolutely necessary? There seems to be a couple of new public functions that could be private from a first glance.

@swfiua
Copy link
Contributor Author

swfiua commented Mar 15, 2017

@NelleV

Re the API: I'm not sure the table.Cell class makes it into the API, I've only ever used axes.table().

Cell.auto_set_font_size() has grown and extra (optional) hint argument, for the font size -- this is partly an attempt to make the font setting do less work by learning from the other cells that have been set already,

I added get_required_dimension() to return required width, height of the cell. Previously the code only cared about the width, so could get by with just get_required_width()

@NelleV
Copy link
Member

NelleV commented Mar 19, 2017

Closing this in favor of #8325

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

Successfully merging this pull request may close these issues.

None yet

5 participants