Skip to content

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

Merged
yutingzhao1991 merged 5 commits into
masterfrom
yu-newlifecycle
May 3, 2018
Merged

refactoring: Migrate to new lifecycle methods, close #288#291
yutingzhao1991 merged 5 commits into
masterfrom
yu-newlifecycle

Conversation

@yutingzhao1991

@yutingzhao1991 yutingzhao1991 commented Apr 20, 2018

Copy link
Copy Markdown
Contributor

@yutingzhao1991

Copy link
Copy Markdown
Contributor Author

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

@codecov-io

codecov-io commented Apr 20, 2018

Copy link
Copy Markdown

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.

@yutingzhao1991
yutingzhao1991 requested a review from yesmeck April 24, 2018 10:15
Comment thread src/DropdownMenu.jsx Outdated
componentWillMount() {
this.lastInputValue = this.props.inputValue;
constructor(props) {
super();

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

super(props)

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

Copy link
Copy Markdown
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
Copy Markdown
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
Copy Markdown
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
Copy Markdown
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.

3 participants