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

Wrapper-type fix for <progress> element not returning to an indeterminate state (#1431) #1443

Closed
wants to merge 5 commits into from
Closed

Wrapper-type fix for <progress> element not returning to an indeterminate state (#1431) #1443

wants to merge 5 commits into from

Conversation

steveluscher
Copy link
Contributor

Here's a wrapper solution to #1431, in case it is decided that a wrapper is the way to go, rather than a mutation method.

Steven Luscher added 2 commits April 23, 2014 17:40
…nsitions from being set to being null, the progress component does not return to the "indeterminate" state (barbershop pole).
…mponent's value is set to null or undefined, the progress element returns to the indeterminate state (barbershop pole).
@danielschonfeld
Copy link
Contributor

I ran into the same problem when the tests ran on Android. Perhaps verifying that node.value == null is sufficient in determining we have achieved success.

@steveluscher
Copy link
Contributor Author

Interesting. Those tests fail on Android, because they target Android 4.0. Android didn't introduce support for the <progress> element until version 4.4.

edbaa9e skips the ReactDOMProgress test if it determines that the <progress> element isn't supported. All green now.

@syranide
Copy link
Contributor

Hmm, I'm curious if a separate wrapper is really needed, after thinking a bit more on it it seems to me like MUST_USE_ATTRIBUTE really is the solution here unless it for some reason isn't compatible, but if that's a problem then it's perhaps ReactDOMInput/Textarea/Select that needs a nudge to avoid bad behavior.

In my mind at least, I'd say the preferred order of potential solutions would be:
MUST_USE_ATTRIBUTE > correct DOMPropertyOperations > mutationMethod > ReactDOMProgress

@zpao @spicyj Side-note, is there a reason why shouldIgnoreValue isn't always true for null/undefined? It seems like we would intuitively want it to clean up after itself if the attribute is no longer used, not that it really matters though perhaps. In keeping with Reacts nature, it would seem like to me that a DOM element should be uniquely defined by it's props (where possible) and not depend on previous values.

@sophiebits
Copy link
Collaborator

Agree that fixing DOMPropertyConfig is preferable to a wrapper.

@syranide Not quite sure what you mean about shouldIgnoreValue, sorry?

@zpao
Copy link
Member

zpao commented Apr 24, 2014

After talking on IRC, keeping this declarative in some way sounds like the best option (maybe making https://gist.github.com/zpao/11268735 or similar work). But in the short term this might be acceptable.

@steveluscher
Copy link
Contributor Author

I would be happy to work on the property override solution; just let me know.

@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!

@sophiebits
Copy link
Collaborator

I don't think we're going to do it this way, so I'm closing this out. Hopefully one of the other solutions will make it in soon.

@sophiebits sophiebits closed this Jun 8, 2014
@steveluscher steveluscher deleted the 1431-indeterminate-progress-fix branch February 16, 2015 18:34
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.

6 participants