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(text-field): put foundation on state and do render input unless foundation is present #353

Merged
merged 15 commits into from
Oct 30, 2018

Conversation

moog16
Copy link

@moog16 moog16 commented Oct 18, 2018

fixes #303

@codecov-io
Copy link

codecov-io commented Oct 18, 2018

Codecov Report

Merging #353 into master will increase coverage by <.01%.
The diff coverage is 93.75%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #353      +/-   ##
==========================================
+ Coverage   96.86%   96.86%   +<.01%     
==========================================
  Files          51       51              
  Lines        1816     1820       +4     
  Branches      210      211       +1     
==========================================
+ Hits         1759     1763       +4     
  Misses         57       57
Impacted Files Coverage Δ
packages/text-field/index.js 100% <100%> (ø) ⬆️
packages/text-field/Input.js 95.06% <87.5%> (+0.06%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 36469f7...f23819c. Read the comment docs.

Copy link
Contributor

@bonniezhou bonniezhou left a comment

Choose a reason for hiding this comment

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

What's the purpose of moving the foundation to state? Semantically foundation seems more like a variable that needs to be initialized rather than one that holds state or can be changed throughout the component's life cycle

@moog16
Copy link
Author

moog16 commented Oct 24, 2018

Well the inner components of a component will be initialized first. In this case, text field initializes before it considers itself "mounted". So the 's componentDidMount is called before text field's componentDidMount. relies on the foundation existing, which only happens after the text field's componentDidMount.

So by adding foundation to state, we can guarantee that the will always have a foundation.

Copy link
Contributor

@bonniezhou bonniezhou left a comment

Choose a reason for hiding this comment

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

Clarification for posterity: foundation is moved to state so that Text Field's componentDidMount will trigger a re-render through setState after initializing the foundation

@googlebot
Copy link

So there's good news and bad news.

👍 The good news is that everyone that needs to sign a CLA (the pull request submitter and all commit authors) have done so. Everything is all good there.

😕 The bad news is that it appears that one or more commits were authored or co-authored by someone other than the pull request submitter. We need to confirm that all authors are ok with their commits being contributed to this project. Please have them confirm that here in the pull request.

Note to project maintainer: This is a terminal state, meaning the cla/google commit status will not change from this state. It's up to you to confirm consent of all the commit author(s), set the cla label to yes (if enabled on your project), and then merge this pull request when appropriate.

@moog16
Copy link
Author

moog16 commented Oct 30, 2018

Test passing, but CLA is not passing because I accidentally merged @TroyTae's commits into this branch. I reverted, so will just merge into master.

@moog16 moog16 merged commit 2cb5d64 into master Oct 30, 2018
@moog16 moog16 deleted the fix/text-field/foundation-on-state branch October 30, 2018 03:40
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.

4 participants