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 Keyboard cannot control month calendar #561

Merged
merged 3 commits into from
May 9, 2019
Merged

Fix Keyboard cannot control month calendar #561

merged 3 commits into from
May 9, 2019

Conversation

susiwen8
Copy link
Contributor

@susiwen8 susiwen8 commented May 6, 2019

This PR relate to ant-design/ant-design#15335
The problem is getDerivedStateFromProps() in MonthPanel.jsx, it will execute right after setState() in setValue(),
https://reactjs.org/docs/react-component.html?utm_source=caibaojian.com#static-getderivedstatefromprops
In getDerivedStateFromProps(), we assign value by the defaultValue in props, but props isn't change, so it won't update, so my solution is move click event out of MonthPanel, put it in CalendarHeader(), just use props to update state.

@susiwen8
Copy link
Contributor Author

susiwen8 commented May 6, 2019

@zombieJ please take a look.

@susiwen8
Copy link
Contributor Author

susiwen8 commented May 6, 2019

Also, I'm thinking, we better use 'value' other than 'defaultValue' in props, cause value in MonthPanel is not default.

newState = {
value: nextProps.value,
value: props.defaultValue,
Copy link
Member

Choose a reason for hiding this comment

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

不能这么改,defaultValue 应该只有第一次生效。

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@afc163 我明白,所以我想把defalutValue变量名改成value, 完全由父组件的控制子组件state

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@afc163 其实组件里defaultValue没有default用途,父组件里会更改defalutValue的值,就是一个可变值,所以变量名应该改成value而不是defaultValue.

Copy link
Member

Choose a reason for hiding this comment

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

@zombieJ 之前改过一次,有点问题,看下 ant-design/ant-design#15335 (comment)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

其实这个问题是setState执行之后,getDerivedStateFromProps会执行一遍(React 16.4之后,之前是props改变才触发),就会覆盖掉setState触发的state变化,这就是为什么我把鼠标点击事件往上层派发,由父组件更改props,键盘控制也是上层组件更改props值来更新子组件的state。

Copy link
Member

Choose a reason for hiding this comment

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

挺蛋疼的,Calendar 里打孔越来越多了。defaultValue 它也不太纯粹,我先本地跑跑。

Copy link
Member

Choose a reason for hiding this comment

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

之前 defaultValue 改成 value 其实是做同样的事情。
感觉既然已经是 state.value 了就直接改成 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.

改了

@afc163 afc163 requested a review from zombieJ May 6, 2019 10:39
@@ -104,6 +112,10 @@ describe('MonthCalendar', () => {
});
expect(wrapper.state().value.month()).toBe(4);
expect(wrapper.state().value.year()).toBe(2017);
const selectedYear = wrapper.find('.rc-calendar-month-panel-year-select-content');
Copy link
Member

Choose a reason for hiding this comment

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

跑了一下,这里 selectedYear 只有一个,为什么需要遍历呢?

@codecov
Copy link

codecov bot commented May 9, 2019

Codecov Report

Merging #561 into master will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@          Coverage Diff           @@
##           master    #561   +/-   ##
======================================
  Coverage    89.7%   89.7%           
======================================
  Files          10      10           
  Lines         709     709           
  Branches      210     210           
======================================
  Hits          636     636           
  Misses         63      63           
  Partials       10      10

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 dcc3a76...d327def. Read the comment docs.

@zombieJ
Copy link
Member

zombieJ commented May 9, 2019

先合了,有问题再看看。

@zombieJ zombieJ merged commit baee788 into react-component:master May 9, 2019
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