Skip to content

Support React 16 #69

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

Merged
merged 1 commit into from
Sep 25, 2017
Merged

Support React 16 #69

merged 1 commit into from
Sep 25, 2017

Conversation

yesmeck
Copy link
Member

@yesmeck yesmeck commented Sep 19, 2017

@coveralls
Copy link

Coverage Status

Coverage increased (+0.6%) to 74.216% when pulling 3a9686d on react-16 into 402016e on master.

@coveralls
Copy link

Coverage Status

Coverage increased (+0.5%) to 74.126% when pulling e74bc84 on react-16 into 402016e on master.

@coveralls
Copy link

Coverage Status

Coverage increased (+0.5%) to 74.126% when pulling ea38de0 on react-16 into 402016e on master.

@coveralls
Copy link

Coverage Status

Coverage increased (+0.5%) to 74.035% when pulling df5caa4 on react-16 into 402016e on master.

@afc163
Copy link
Member

afc163 commented Sep 20, 2017

@benjycui ...

@yesmeck
Copy link
Member Author

yesmeck commented Sep 20, 2017

@benjycui

@benjycui
Copy link
Member

我也是刚看到。。

src/index.js Outdated
portal = createPortal(this.getComponent(), this._container);
}

return [
Copy link
Member

Choose a reason for hiding this comment

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

要同时兼容 React@15 和 React@16,所以不能直接返回一个 fragment

Copy link
Member Author

Choose a reason for hiding this comment

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

看上面👆,15 的话会直接返回。

src/index.js Outdated
let portal;
// prevent unmounting when visible change to false
if (popupVisible || this._component) {
this._container = this._container || getContainer(this);
Copy link
Member

Choose a reason for hiding this comment

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

@afc163 这个问题还是存在的。。

image

@yesmeck
Copy link
Member Author

yesmeck commented Sep 20, 2017

Updated.

@coveralls
Copy link

Coverage Status

Coverage increased (+0.6%) to 74.138% when pulling ae79e53 on react-16 into 010d15d on master.

@coveralls
Copy link

Coverage Status

Coverage increased (+0.6%) to 74.227% when pulling 0d16889 on react-16 into 010d15d on master.

@yesmeck yesmeck force-pushed the react-16 branch 2 times, most recently from f893630 to 541ed9c Compare September 20, 2017 07:16
@coveralls
Copy link

Coverage Status

Coverage increased (+0.6%) to 74.227% when pulling f893630 on react-16 into 010d15d on master.

@coveralls
Copy link

Coverage Status

Coverage increased (+0.6%) to 74.227% when pulling 541ed9c on react-16 into 010d15d on master.

@coveralls
Copy link

Coverage Status

Coverage increased (+0.6%) to 74.227% when pulling 3e4f45b on react-16 into 010d15d on master.

}

createContainer() {
this._container = this.props.getContainer();
Copy link
Member

Choose a reason for hiding this comment

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

@afc163 FYI,getPopupContainer 之类的 API,其实只会调用一次,并且之前一直就是这样。考虑到有 removeContainer 的需要,getPopupContainer 的确只能调用一次。

Copy link
Member

Choose a reason for hiding this comment

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

没懂,有啥问题。

如果父元素 dom 结构变了,可能需要调用多次吧。

Copy link
Member

Choose a reason for hiding this comment

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

不缓存 getPopupContainer 的返回值其实也可以,只是每次 render 都需要对比当前返回值和之前的返回值,并相应的 mount unmount 节点。

Copy link
Member

Choose a reason for hiding this comment

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

并且这样就要求用户的 getPopupContainer 必须稳定,不能出现以下的情况:

getPopupContainer() {
  const div = new div
  document.appendChild(div);
  return div;
}

Copy link
Member

Choose a reason for hiding this comment

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

这样就是 breaking changes 了。

Copy link
Member Author

Choose a reason for hiding this comment

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

这个先保持原有的行为吧。

@benjycui benjycui requested review from yiminghe and afc163 September 21, 2017 01:50
@benjycui
Copy link
Member

没问题的话,下周一合并发布个 2.0 吧,rc-menu 这边直接用就好。

@benjycui benjycui merged commit f970ed0 into master Sep 25, 2017
@benjycui benjycui deleted the react-16 branch September 25, 2017 05:47
@benjycui
Copy link
Member

[email protected]

yesmeck referenced this pull request in react-component/m-dialog Sep 28, 2017
@yesmeck
Copy link
Member Author

yesmeck commented Sep 29, 2017

这里引入了另外一个问题,因为要第二次 render 才能真正渲染 popup 的内容,导致第一次 componentDidMount 的时候会拿不到 popup 里内容的 ref。例子:

class Demo extends React.Component {
  componentDidMount() {
    console.log(this.box); // undefined
  }

  render() {
    const popup = (
      <div ref={node => this.box = node}>hello</div>
    );

    return (
      <Trigger popupVisible={true} popup={popup}>
        <div>hover me</div>
      </Trigger>
    )
  }
}

@benjycui
Copy link
Member

可以加个 warning?如果没有其他办法的话。。

@silentcloud
Copy link
Member

silentcloud commented Sep 29, 2017

setTimeout 终极大法可以的

componentDidMount() {
  setTimeout(() => {
    console.log(this.box); 
  }, 0);
}

先这么处理,后面升个 break change

@benjycui
Copy link
Member

setTimeout 终极大法可以的

问题是用户会困惑。

@yesmeck
Copy link
Member Author

yesmeck commented Sep 29, 2017

setTimeout 太挫了。

@silentcloud
Copy link
Member

silentcloud commented Sep 29, 2017

这个没办法,后面 去掉 getContainer 里 findDOMNode,那个回传参数没用 ;

setTimeout 太挫了。

是的,只是目前这种的解决方法了

@yesmeck
Copy link
Member Author

yesmeck commented Sep 29, 2017

想了下去掉 findDOMNode 也不行,因为 getContainer 的元素可能是父组件里的某个元素,那么如果在 rendercreateContainer 的话,那个元素其实还没渲染出来。

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.

5 participants