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

Fix/12200 regresssion select multiple #12240

Conversation

GarethSmall
Copy link
Contributor

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

@pull-bot
Copy link

Details of bundled changes.

Comparing: e68c016...b95667e

react-art

File Filesize Diff Gzip Diff Prev Size Current Size Prev Gzip Current Gzip ENV
react-art.production.min.js 0% -1% 49.56 KB 49.67 KB 15.63 KB 15.62 KB NODE_PROD

Generated by 🚫 dangerJS

@gaearon
Copy link
Collaborator

gaearon commented Feb 17, 2018

Why is a separate hook necessary?

@GarethSmall
Copy link
Contributor Author

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?

@aweary
Copy link
Contributor

aweary commented Feb 17, 2018

I think one of the other two solutions I listed in #12200 (comment) would be preferable, but I'd like @gaearon's input.

@gaearon
Copy link
Collaborator

gaearon commented Feb 17, 2018

I think it's more likely we can change the order of the existing hooks than that we'd introduce a new hook.

@GarethSmall
Copy link
Contributor Author

Do you mean by switching appendAllChildren and finalizeInitialChildren?

@gaearon
Copy link
Collaborator

gaearon commented Feb 17, 2018

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.

@GarethSmall
Copy link
Contributor Author

Alright, thank you for the input. Im going to head back to the drawing board on this.

@gaearon
Copy link
Collaborator

gaearon commented Feb 17, 2018

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.

@GarethSmall
Copy link
Contributor Author

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.

@gaearon
Copy link
Collaborator

gaearon commented Aug 3, 2018

For posterity, the fix was #13270

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

Successfully merging this pull request may close these issues.

None yet

5 participants