Skip to content
This repository was archived by the owner on Aug 11, 2025. It is now read-only.

Conversation

@ChanTsune
Copy link
Contributor

Thanks for the wonderful library!

I wanted to use multiple SVG images inline in HTML.

But, currently generated SVG element id attribute is always "smooth" and "round".

Using multiple SVG images inline in HTML causes display issues due to duplicate id attributes.

So I made it possible to add a suffix to the id attribute.

@google-cla
Copy link

google-cla bot commented Sep 26, 2020

Thanks for your pull request. It looks like this may be your first contribution to a Google open source project (if not, look below for help). Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA).

📝 Please visit https://cla.developers.google.com/ to sign.

Once you've signed (or fixed any issues), please reply here with @googlebot I signed it! and we'll verify it.


What to do if you already signed the CLA

Individual signers
Corporate signers

ℹ️ Googlers: Go here for more info.

@ChanTsune
Copy link
Contributor Author

@googlebot I signed it!

Copy link
Contributor

@brianquinlan brianquinlan left a comment

Choose a reason for hiding this comment

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

Very nice change, thanks!

whole_title: Optional[str] = None,
left_title: Optional[str] = None,
right_title: Optional[str] = None,
id_suffix: str = '',
Copy link
Contributor

Choose a reason for hiding this comment

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

How about Optional[str] = None and then we have some code like:

if id_suffix is None:
  id_suffix = str(uuid.uuid4())

?

What do you think? That would mean that the tests need to be updated and the major version of the library bumped?

Or do you think that it is better to leave the default?

Copy link
Contributor Author

@ChanTsune ChanTsune Oct 17, 2020

Choose a reason for hiding this comment

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

Thank you for your review!

I think adding a random string seems like a good suggestion.

However, the W3C state that:

they cannot start with a digit, two hyphens, or a hyphen followed by a digit.

https://www.w3.org/TR/CSS21/syndata.html#characters.

When using str(uuid.uuid4()), there are cases where a number exists after -.

>>> str(uuid4())
'e489d2a0-39c3-4508-94c5-8337581da1a3'

So I think it's better to remove the - or leave it as it is.

Also, if keep the current PR, you don't have to update the major version, which is a merit I think.

@brianquinlan brianquinlan merged commit 1f4de87 into google:master Nov 7, 2020
@ChanTsune ChanTsune deleted the feature/add-hash-to-id branch November 7, 2020 01:58
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants