Skip to content
This repository has been archived by the owner on Jul 19, 2019. It is now read-only.

Pass props.id through to the rendered Surface tag #40

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

jesstelford
Copy link

This allows setting the element's id tag.

Note: I couldn't run the build or test suites - it appears gulp and jest aren't part of the package.json, and I received errors when attempting to install them.

This allows setting the element's id tag.
@facebook-github-bot
Copy link

Thank you for your pull request and welcome to our community. We require contributors to sign our Contributor License Agreement, and we don't seem to have you on file. In order for us to review and merge your code, please sign up at https://code.facebook.com/cla - and if you have received this in error or have any questions, please drop us a line at [email protected]. Thanks!

@jesstelford
Copy link
Author

Haha, what? Are you serious? For adding a single line of code... Seems way too excessive :(

edit I have signed the agreement. Any further action required by me?

@facebook-github-bot
Copy link

Thank you for signing our Contributor License Agreement. We can now accept your code for this (and any) Facebook open source project. Thanks!

@sebmarkbage
Copy link
Contributor

May I ask what you need the id for? The idea is that an ART surface is an opaque element and you don't know what kind of tag it renders to. The output mode may need to use the "id" attribute for its own purposes. For example, in SVG ids are frequently used to refer to elements declared within defs etc. so the underlying engine may need to utilize that ID for something else.

@jesstelford
Copy link
Author

May I ask what you need the id for?

I wanted to target the containing element in CSS, to float it for example. I was hoping to do that without extra wrapping markup, or relying on the rendered tag (due to being unknown).

SVG ids are frequently used to refer to elements declared within defs

Is this true for the root element of an SVG document? I don't want to propagate the id internally into the SVG/VML document, just target the root.

@sebmarkbage
Copy link
Contributor

It is not used for the root element at the moment. Not sure if we'll ever need that, so I just want to make sure we don't introduce something we may have to break later.

If you're targeting it through CSS, wouldn't it be more appropriate to target it using className which is already supported?

Although, even className is kind of fragile since a lot of the CSS is overridden on the surface by inline styles anyway:

https://github.com/sebmarkbage/art/blob/master/modes/vml/dom.js#L20

https://github.com/sebmarkbage/art/blob/master/modes/canvas/surface.js#L85-94

The float attribute currently isn't, but I'm not sure we can guarantee that it will work cross-browser since we do override the display style attribute which is related. Perhaps float on the surface will cause things to break in edge-cases. I don't know.

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

Successfully merging this pull request may close these issues.

3 participants