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 [prop='foo'] CSS syntax #4

Closed
lelandrichardson opened this issue Nov 11, 2015 · 18 comments
Closed

Add [prop='foo'] CSS syntax #4

lelandrichardson opened this issue Nov 11, 2015 · 18 comments

Comments

@lelandrichardson
Copy link
Collaborator

No description provided.

@lelandrichardson
Copy link
Collaborator Author

Just an update: it looks like https://github.com/chrisdickinson/cssauron could potentially be of use here...

@blainekasten
Copy link
Contributor

Could this extend to event handler keys? Like if I want to find the element that has my click handler. ie,

instance.find('[onClick]');

@lelandrichardson
Copy link
Collaborator Author

@blainekasten I'm hoping we can extend it beyond CSS in a way that makes sense and is useful without bastardizing the CSS spec. '[onClick]' and support for any custom prop names definitely makes sense to me as a suggestion!

@blainekasten
Copy link
Contributor

Awesome. Is anyone internally working on this one yet? If not, I may take a stab at it.

@lelandrichardson
Copy link
Collaborator Author

@blainekasten not yet. Stab away!

@ljharb
Copy link
Member

ljharb commented Dec 11, 2015

It might also be nice to have a different syntax, or a different method, for event handlers, so an actual HTML attribute onClick won't collide with a react onClick handler?

@blainekasten
Copy link
Contributor

Do you have a use case where someone uses the browsers onclick handler? I don't think React is meant to be used in that way, because then you lose the benefits of event delegation and pooling.

@ljharb
Copy link
Member

ljharb commented Dec 11, 2015

No, of course I don't have a use case :-) but the fact that it may conflict is a good reason not to create a future footgun, when there's lots of syntax options available that aren't otherwise valid querySelector syntax.

@blainekasten
Copy link
Contributor

Thinking about it though, since they are just keys of an object (props). They wouldn't clash.

You could do either '[onClick'] for the react binding, or '[onclick'] for the native binding.

@lelandrichardson
Copy link
Collaborator Author

I agree. I don't think there's any concern of conflict here, unless I'm missing something...

@ljharb
Copy link
Member

ljharb commented Dec 11, 2015

[] is HTML attribute selector syntax. if it's not selecting HTML attributes, it's not the right syntax for it. Why not {onClick} or something?

@blainekasten
Copy link
Contributor

I see what you are saying. It's your API so I think that is decision you guys should probably make. I could see arguments for both.

On one side, since react is modeling the DOM for us, you could think of props being attributes. Especially from a user perspective who is used to html, it looks like they are writing attributes, so they might guess that the [] is the right API for finding the elements for any react prop.

On the other side, {} separates it so you can be more specific and intentional with what a user is trying to do. It could also future proof (as @ljharb mentioned) to new APIs specific to React and make sure we don't clash with a normal html attribute. Although I think they are good at doing that themselves. I would be very surprised if they had a react prop be the same key as an html attribute.

@ljharb
Copy link
Member

ljharb commented Dec 11, 2015

What would [htmlFor] select? Should it select the "for" attribute, or the htmlFor prop? I feel like it's a dangerous precedent to make selector strings not be either copy-pasteable to qSA so they work the same, or, copy-pasteable so they don't silently fail when used outside of enzyme. Using alternative syntax would cause an error to be thrown if enzyme-specific syntax is used outside of enzyme.

I totally agree it's very unlikely this would become a problem - but it's a trivial thing to prevent before the feature exists, and a much harder thing to clean up if it does become a problem later.

@blainekasten
Copy link
Contributor

So, i've been working on this for a day now. I have the property look up of the react node working in shallow rendering. So I either need some guidance on this to find out how to do this, or we're not going to get the API @ljharb is hoping for.

Basically, it seems that in shallow rendering we aren't getting access to a rendered version of the component. If that is the case, we can't do [] for attributes and {} for props because we can't get the html attributes.

I guess I could mock it out somehow with a map. But that might get huge and unwieldly. I guess all this to say, I can make it work if that is the API desired. I personally don't think I would ever need a time where I have to query on the html attribute over the react prop. But I understand your argument also.

@lelandrichardson
Copy link
Collaborator Author

I guess I'm still not sure I see @ljharb's point... which is not to say that it's not a valid one... I'm just not convinced I see a conflict at this point.

From the way I see it, we aren't querying HTML nodes... we are querying react nodes. the css [foo] selector is more or less taken from XPATH which is intended to work on all XML-like things... in our case this is JSX...

In the question that @ljharb posed:

What would [htmlFor] select? Should it select the "for" attribute, or the htmlFor prop?

To me it seems the answer is easy: it would find react nodes where the htmlFor prop was defined. There are no nodes with the for attribute defined... as react does not allow it, and if it did allow it, it would be selected for by using [for]. I can't think of a scenario where there would ever be a collision or conflict?

Just like the #foo selector matches all nodes where the id prop is equal to "foo" and the .foo selector matches all nodes where the className prop contains "foo" (whitespace delimited).

@blainekasten if you think you have a working PR ready to handle one of these two situations, maybe put it up and we can continue the discussion there with a bit more context regarding the solution?

@blainekasten
Copy link
Contributor

#70 is up for discussion.

@0xR
Copy link

0xR commented Dec 16, 2015

+1 for attributes selectors, for what it's worth. I'm not migrating to enzyme for now because of this missing feature.

@lelandrichardson
Copy link
Collaborator Author

Closing as this got merged and is now available in 1.3.x

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

No branches or pull requests

4 participants