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

update react refs to use callback instead of strings #282

Merged
merged 3 commits into from
Jul 27, 2017

Conversation

coderpawz
Copy link
Contributor

As of React v16 using strings to create refs will no longer work. This is is forward looking change to address future compatibility issues as well as fixing existing deprecation warning thrown by react v15.

For more info on refs, see https://facebook.github.io/react/docs/refs-and-the-dom.html

@coveralls
Copy link

Coverage Status

Coverage increased (+0.1%) to 77.612% when pulling 74da6c4 on coderpawz:master into c3780a0 on react-component:master.

@HipsterZipster
Copy link

This is a vital PR that unblocks devs that are using React 16, along with react-component/select#212 and react-component/trigger#63

@afc163 can we please get these merged asap?

src/Calendar.jsx Outdated
@@ -202,6 +202,9 @@ const Calendar = createReactClass({
showTimePicker: false,
});
},
saveDateInput(dateInput) {
this.dateInputInstance = dateInput;
Copy link
Member

Choose a reason for hiding this comment

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

If this ref is not going to be used, just remove it.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

removed, thanks for taking a look

@coveralls
Copy link

Coverage Status

Coverage increased (+0.05%) to 77.564% when pulling e8c9a7b on coderpawz:master into c3780a0 on react-component:master.

@@ -413,6 +413,10 @@ const RangeCalendar = createReactClass({
return month.isSameOrBefore(value[0], 'month');
},

saveRoot(root) {
Copy link
Member

Choose a reason for hiding this comment

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

can we move this saveRoot function to commonMixin ? since this.rootInstance is used in commonMixin too.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

moved, had to remove it from CalendarMixin as well
thanks!

@paranoidjk
Copy link
Member

@benjycui

  maintainers:
   [ 'benjycui <benjytrys@gmail.com>',
     'yiminghe <yiminghe@gmail.com>' ],

@paranoidjk
Copy link
Member

@benjycui npm owner add paranoidjk

remove saveRoot from Calendar and CalendarMixin
@coveralls
Copy link

Coverage Status

Coverage remained the same at 77.516% when pulling 683cb04 on coderpawz:master into c3780a0 on react-component:master.

@benjycui
Copy link
Member

$ npm owner ls rc-calendar
paranoidjk <hust2012jiangkai@gmail.com>
benjycui <benjytrys@gmail.com>
yiminghe <yiminghe@gmail.com>

@benjycui
Copy link
Member

$ npm info rc-calendar
...
 maintainers:
   [ 'benjycui <benjytrys@gmail.com>',
     'yiminghe <yiminghe@gmail.com>' ],
...

@benjycui
Copy link
Member

It seems that it is a bug of npm, just try to publish first.

@paranoidjk paranoidjk merged commit 4bf77ab into react-component:master Jul 27, 2017
@yesmeck
Copy link
Member

yesmeck commented Jul 30, 2017

= = 这个就直接升 major 了啊。

@paranoidjk
Copy link
Member

@yesmeck 不敢保证是否有用户会使用我们内部的 ref,稳妥起见这边升 major。

ant-design/ant-design#3979 (comment)

这不是你自己写的么 😅

@yesmeck
Copy link
Member

yesmeck commented Jul 30, 2017

是的,但是这个不是公开的 API。

@yesmeck
Copy link
Member

yesmeck commented Jul 30, 2017

这里升 major 的话 antd 那边就很尴尬啊, antd 也得升 major?

@paranoidjk
Copy link
Member

paranoidjk commented Jul 30, 2017

antd 我感觉不用, antd 更上层, api 和使用文档什么的相对比较固定和清晰 ant-design/ant-design@bcb82a5

rc-component 这层严格保证 semver,牵扯面太广了

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.

6 participants