Skip to content
This repository was archived by the owner on Aug 19, 2022. It is now read-only.

[WIP] clear radium state for unmounted components#955

Closed
stefvhuynh wants to merge 3 commits intomasterfrom
hover-state
Closed

[WIP] clear radium state for unmounted components#955
stefvhuynh wants to merge 3 commits intomasterfrom
hover-state

Conversation

@stefvhuynh
Copy link
Copy Markdown
Contributor

Initial attempt to resolve #524.

  1. Keep track of children keys when going through them recursively in resolve-styles.js
  2. Check these keys against the state in enhancer.js
  3. Remove the keys in the state that correspond with unmounted components in enhancer.js

There is still a lot of cleanup to do, but I'm looking for opinions on the approach.

@stefvhuynh
Copy link
Copy Markdown
Contributor Author

@ryan-roemer, @ianobermiller, this is an initial attempt at resolving this issue. Can I get thoughts on the approach?

@coveralls
Copy link
Copy Markdown

coveralls commented Jan 3, 2018

Coverage Status

Coverage decreased (-1.0%) to 93.056% when pulling 25786b9 on hover-state into 2c4ae5e on master.

1 similar comment
@coveralls
Copy link
Copy Markdown

Coverage Status

Coverage decreased (-1.0%) to 93.056% when pulling 25786b9 on hover-state into 2c4ae5e on master.

@alexlande
Copy link
Copy Markdown
Contributor

General approach looks solid. I'm a little nervous about the check in componentDidUpdate because it could be called extremely frequently in some cases (like if you're doing animations), but I can't think of a better way to handle it.

@stefvhuynh
Copy link
Copy Markdown
Contributor Author

@alexlande, another possible approach would be to expose setState on radium as suggested by some people in the issue. though, it seems like it would make sense to clear style states if a component is removed anyway.

@alexlande
Copy link
Copy Markdown
Contributor

@stefvhuynh Yeah, it doesn't feel like something that users should have to handle, if we can do it automatically.

@stefvhuynh
Copy link
Copy Markdown
Contributor Author

@alexlande, alright, i'll clean this up then.

@stefvhuynh
Copy link
Copy Markdown
Contributor Author

continued in #956

@stefvhuynh stefvhuynh closed this Jan 5, 2018
@stefvhuynh stefvhuynh deleted the hover-state branch January 5, 2018 22:33
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Elements that are hidden when in the hover state come back with the hover state active

3 participants