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

Add title tag #24

Merged
merged 4 commits into from
Oct 7, 2020
Merged

Add title tag #24

merged 4 commits into from
Oct 7, 2020

Conversation

matthewturk
Copy link
Contributor

This adds a "title" tag, which can be used as described in the answer here:

https://stackoverflow.com/questions/10643426/how-to-add-a-tooltip-to-an-svg-graphic

to add a tooltip. For instance,

g = draw.Group(id = f"cell_{n:02d}")
g.append(draw.Title(f"cell: {i: 2d} {j: 2d}"))

Copy link
Owner

@cduck cduck left a comment

Choose a reason for hiding this comment

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

Thanks for the pull request. This is a good feature to add.

As it is, the new Title element can't be used with most of the drawing elements (e.g. Rectangle doesn't have an append method). I suggest adding an appendTitle method to DrawingBasicElement similar to appendAnim. Something like

    def appendTitle(self, text, **kwargs):
        self.children.append(Title(text, **kwargs))

See my comments to improve code quality.

Also, please add this to the examples. I don't think it needs its own example so I would just add a title to the rectangle in example 1 (with a code comment saying what it does).

drawSvg/elements.py Outdated Show resolved Hide resolved
drawSvg/elements.py Outdated Show resolved Hide resolved
This updates based on the comments in the pull request, and updates the
example SVG as well.
@matthewturk
Copy link
Contributor Author

@cduck Thanks for the comments! I think I addressed everything you brought up, but I wasn't totally sure if I chose the right name for the parent class. Does this align with what you're thinking?

Copy link
Owner

@cduck cduck left a comment

Choose a reason for hiding this comment

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

Thanks. This looks good. Just two minor changes then I'll merge it.

drawSvg/elements.py Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
drawSvg/elements.py Outdated Show resolved Hide resolved
@matthewturk
Copy link
Contributor Author

Thank you!

''' A line of text within the Text element. '''
TAG_NAME = 'tspan'
class _TextContainingElement(DrawingBasicElement):
''' A private parent class used for elements that only have plain text content. '''
Copy link
Owner

Choose a reason for hiding this comment

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

Forgot to wrap to 80 characters.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Whoops, sorry for really drawing this review out into tedium ...

Copy link
Owner

Choose a reason for hiding this comment

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

No problem. This only happens because I haven't added continuous integration to the project yet.

@matthewturk
Copy link
Contributor Author

And with my last commit, a careless vim keystroke has included my command into the commit message. I'm happy to either rebase and squash or whatever works for you, since I've really shown my clumsiness here.

@cduck
Copy link
Owner

cduck commented Oct 7, 2020

I squash all PRs when I merge and usually use the PR description as the final commit message.

@matthewturk
Copy link
Contributor Author

matthewturk commented Oct 7, 2020 via email

@cduck cduck changed the title Add a title tag Add title tag Oct 7, 2020
@cduck cduck merged commit 733e0bd into cduck:master Oct 7, 2020
@cduck
Copy link
Owner

cduck commented Oct 7, 2020

Merged and published version 1.7.0 (https://pypi.org/project/drawSvg/1.7.0/). Thanks for your help.

@matthewturk matthewturk mentioned this pull request Oct 8, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants