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

Injectable DOM properties #141

Closed
wants to merge 6 commits into from
Closed

Conversation

petehunt
Copy link
Contributor

#140 was reported today which was something I was interested in fixing anyway. Turns out this is pretty easy.

During the course of this diff I noticed we didn't expose the id attribute; this is fixed. I use MUST_USE_PROPERTY per http://stackoverflow.com/questions/4851699/setting-the-id-attribute-of-an-input-element-dynamically-in-ie-alternative-for.

I think this internal refactoring is basically pure win. If you guys don't want to expose the API publicly yet to keep the surface area of the API small I can revert the second commit on this.

If accepted into the public API, I will document this. I think this is definitely a rough edge that people will run into. Also in the future we can reduce our byte size by making all the svg props optional.

action: null,
ajaxify: DOMProperty.injection.MUST_USE_ATTRIBUTE,
allowFullScreen: DOMProperty.injection.MUST_USE_ATTRIBUTE |
DOMProperty.injection.HAS_BOOLEAN_VALUE,
Copy link
Contributor

Choose a reason for hiding this comment

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

Nit: can you store these as local variables at the top to make this code pretty again? :)

allowFullScreen: MustUseAttribute | HasBooleanValue,

* Mapping from normalized, camelcased property names to a configuration that
* specifies how the associated DOM property should be accessed or rendered.
*/
MUST_USE_ATTRIBUTE: 0x1,
Copy link
Member

Choose a reason for hiding this comment

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

Octal literals are deprecated in es5 but it looks like hex still works. I guess it doesn't actually matter but I would consider using parseInt...

EDIT: realize now that this was already there, just shuffled

@zpao
Copy link
Member

zpao commented Jul 1, 2013

2 things that I think would need answers before we can merge:

  • What do we do about collisions? injectDOMPropertyConfig will overwrite properties (so last wins) but it will append isCustomAttribute which will short-circuit. This feels a little inconsistent and will need to be well documented.
  • Do we want to be able to uninject (eject?) a config?

@petehunt
Copy link
Contributor Author

petehunt commented Jul 3, 2013

I did this upstream. We decided to not expose the API publicly yet since we have a few unanswered questions. But at least now the core is injectable such that it's easy to build an API for this in the future.

@petehunt petehunt closed this Jul 3, 2013
@zpao
Copy link
Member

zpao commented Jul 3, 2013

The upstream commit synced back down: 32423a8

@kirbysayshi
Copy link

Sorry to bump an old issue, but is there any chance this will be exposed? I'd like to use a jsx template like:

<div dataCustomUri="{this.props.uri}"></div>

And then use it for event delegation later:

delegate(document, '[data-custom-uri]', function(e) { ... });

But right now react ignores all data-*, which would be pretty easily fixed with the ability to add in a custom DOMPropertyConfig.

@petehunt
Copy link
Contributor Author

@kirbysayshi You should just be able to do <div data-custom-uri={this.props.uri} /> and it'll work. data- attributes are special cased.

@kirbysayshi
Copy link

You are correct! It looks like I had extra " around the value...

data-custom-uri="{this.props.uri}"

vs

data-custom-uri={this.props.uri}

Sorry to bother.

andreypopp pushed a commit to andreypopp/jsxx that referenced this pull request Feb 2, 2014
toptaldev92 pushed a commit to toptaldev92/react_project that referenced this pull request Jul 28, 2021
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.

4 participants