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

refactoring: Migrate to new lifecycle methods, close #288 #291

Merged
merged 5 commits into from May 3, 2018

Conversation

yutingzhao1991
Copy link
Contributor

@yutingzhao1991 yutingzhao1991 commented Apr 20, 2018

@yutingzhao1991
Copy link
Contributor Author

有种把 vue 里面 watch 改为 computed 的感觉...

@codecov-io
Copy link

codecov-io commented Apr 20, 2018

Codecov Report

Merging #291 into master will decrease coverage by 0.01%.
The diff coverage is 100%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #291      +/-   ##
==========================================
- Coverage   99.66%   99.64%   -0.02%     
==========================================
  Files           8        8              
  Lines         297      285      -12     
  Branches       76       74       -2     
==========================================
- Hits          296      284      -12     
  Misses          1        1
Impacted Files Coverage Δ
src/Select.jsx 100% <100%> (ø) ⬆️
src/DropdownMenu.jsx 100% <100%> (ø) ⬆️

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 72b0fc3...34f49c6. Read the comment docs.

componentWillMount() {
this.lastInputValue = this.props.inputValue;
constructor(props) {
super();
Copy link
Member

Choose a reason for hiding this comment

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

super(props)

src/Select.jsx Outdated
open,
optionsInfo,
// mark defaultValue has been setted, propr.defaultValue only work at the first time.
canUseDefaultValue: true,
Copy link
Member

Choose a reason for hiding this comment

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

this.state = {
  value: props.defaultValue
}

就不需要 canUseDefaultValue 了吧。

Copy link
Contributor Author

Choose a reason for hiding this comment

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

getDerivedStateFromProps 这个方法在初始化和 props 变化的时候都会被调用,为了把赋值的相关逻辑都统一放到那里面去处理,所以加了 canUseDefaultValue。理论上也是可以把 defaultValue 相关的逻辑放到 constructor 里面的,但是有的逻辑(主要是 inputValue 初始化的逻辑)就需要在 constructor 和 getDerivedStateFromProps 里面都写一下了。

Copy link
Member

Choose a reason for hiding this comment

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

constructor 里用 defaultValue 来初始化 value,然后后面都是用 prevState.value吧,value = 'value' in nextProps.value ? nextProps.value : prevState.value 这样。

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done

@yutingzhao1991 yutingzhao1991 merged commit dd4f367 into master May 3, 2018
@yutingzhao1991 yutingzhao1991 deleted the yu-newlifecycle branch May 3, 2018 08:21
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.

None yet

3 participants