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

Change Text's use of deprecated lifecycle #576

Merged
merged 1 commit into from
Jan 11, 2020

Conversation

diagramatics
Copy link
Contributor

@diagramatics diagramatics commented Jan 4, 2020

🏠 Internal

  • [text] Remove deprecated lifecycles used in Text

Fixes #396. Closes #431. I thought I might as well take a crack at converting the deprecated lifecycles to something better. I was gonna use hooks but I have no idea if I can use it yet.

@williaster
Copy link
Collaborator

Nice, thanks for the PR @diagramatics! Code looks good to me but I will pull the branch to test it a bit. Meanwhile @techniq if you have thoughts let us know 👍

We currently have not added hooks to enable compatibility with more react versions, but hope to do so soon after we land the TypeScript project.

@techniq
Copy link
Collaborator

techniq commented Jan 6, 2020

@diagramatics @williaster LGTM, but it's been a while since I wrote that to remember all the nuances (and have been using hooks / useEffect for too longer to remember all the nuances of the lifecycle methods 🤦‍♂). I think if it tests out will with the demo we should be good.

I figure when we switch to hooks we would want to use useLayoutEffect.

@diagramatics
Copy link
Contributor Author

Don't worry, I'm on the same boat here. I had to re-read the docs to figure out 😂

useLayoutEffect would be what we will reach for, yes.

Copy link
Collaborator

@williaster williaster left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry for the delay! I've been swamped trying to get through #579. I was able to test this locally and the demo still looks good 👍 going to go ahead and merge it, it should come out as part of 0.0.193 which depends on #579 wrapping up to test the robustness/usability of the TS types.

@williaster williaster merged commit 942d440 into airbnb:master Jan 11, 2020
@williaster williaster added this to the v0.0.193 milestone Jan 11, 2020
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.

using strict with vx/text
3 participants