-
Notifications
You must be signed in to change notification settings - Fork 47.1k
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
Fix/12200 regresssion select multiple #12240
Fix/12200 regresssion select multiple #12240
Conversation
…rties / Implemented setIntialSpecialProperties
Details of bundled changes.Comparing: e68c016...b95667e react-art
Generated by 🚫 dangerJS |
Why is a separate hook necessary? |
The idea was to set select attributes before the children were appended. Currently it goes append children > set attributes. Which for this is an issue. Maybe an extra hook was overkill in this instance. Do you have an alternative solution? |
I think one of the other two solutions I listed in #12200 (comment) would be preferable, but I'd like @gaearon's input. |
I think it's more likely we can change the order of the existing hooks than that we'd introduce a new hook. |
Do you mean by switching appendAllChildren and finalizeInitialChildren? |
Sorry, I don't remember how this works, and don't anticipate having time to look into this in the near future. I'm just saying that introducing a new hook for all renderers just to fix an edge case in DOM sounds a bit like overkill. If we can show this is valuable for more than a single case then maybe. |
Alright, thank you for the input. Im going to head back to the drawing board on this. |
I'm not saying this option isn't available as a last resort or if you make a compelling case for why it might be necessary universally. Just that an API addition (even an internal one) needs more justification than a bugfix. |
Initially i tested out switching the hooks unfortunately this introduced more bugs than the one fix. I thought this might be a bit excessive for one case. Ill try out a few more possible solutions for this issue and see what I come up with. |
For posterity, the fix was #13270 |
Problem
When creating a select element and setting multiple the first element would be selected. This was due to the multiple prop being set after the children we're appended.
An non-react example case created by @aweary can been seen here - https://jsfiddle.net/dm6vkq9q/
Solution
Have a special case where if the element type were of select set the properties before the children are appended.
Issue
#12200