Skip to content

Conversation

judfs
Copy link
Contributor

@judfs judfs commented Nov 29, 2023

PR summary

Explains what the extents are. Addresses my problem here https://discourse.matplotlib.org/t/spanselector-how-to-set-selection-with-code/24114/4

Old:

https://matplotlib.org/stable/api/widgets_api.html#matplotlib.widgets.SpanSelector.extents

New rendering:

image

PR checklist

Co-authored-by: hannah <story645@gmail.com>
@story645
Copy link
Member

sorry my suggestion made the line too long so you'll have to fix that

@story645 story645 added this to the v3.8.3 milestone Nov 29, 2023
@story645 story645 added the Documentation: API files in lib/ and doc/api label Nov 29, 2023
@@ -2695,7 +2695,7 @@ def _press(self, event):

@property
def direction(self):
"""Direction of the span selector: 'vertical' or 'horizontal'."""
"""Direction of the span selector: 'vertical' or 'horizontal'. Writable."""
Copy link
Member

Choose a reason for hiding this comment

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

What are you trying to communicate w/ writable?

Copy link
Contributor Author

@judfs judfs Nov 30, 2023

Choose a reason for hiding this comment

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

The property has a setter. When everything else in matplotlib is set_title() or whatever, I don't take that for granted.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

No guidance here for if properties were actually used. https://matplotlib.org/devdocs/devel/document.html#setters-and-getters

Copy link
Member

Choose a reason for hiding this comment

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

I think @timhoffm has strong opinions and an open PR on documenting properties #22699

Copy link
Member

Choose a reason for hiding this comment

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

@judfs can you remove the writable? I don't think it's conveying what you intend to here and I don't want to hold up your PR longer

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Changed 👍 . As a user of mpl, I still would find it non obvious that the property is read/write / settable, but that is a more general issue. For now glad to have what the extents are described.

@story645 story645 merged commit b19a5b8 into matplotlib:main Dec 5, 2023
meeseeksmachine pushed a commit to meeseeksmachine/matplotlib that referenced this pull request Dec 5, 2023
@story645
Copy link
Member

story645 commented Dec 5, 2023

Thanks for the contribution! I think there's follow up here, both on how we communicate that something is a property and probably similar to #27200 about adding an extents role to the one place we define extents.

story645 added a commit that referenced this pull request Dec 5, 2023
…397-on-v3.8.x

Backport PR #27397 on branch v3.8.x (SpanSelector widget: Improve doc for `extents`)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Documentation: API files in lib/ and doc/api
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants