feat: allow overflow in horizontal mode #159

picodoth · 2018-06-19T14:57:58Z

addressing #148

TODO:

Adding test

picodoth · 2018-06-19T14:59:46Z

src/MenuItem.jsx

-      });
+      // scrollIntoView(ReactDOM.findDOMNode(this), ReactDOM.findDOMNode(this.props.parentMenu), {
+      //   onlyScrollIfNeeded: true,
+      // });


@yesmeck do we still need this with the new feature?

picodoth · 2018-06-19T15:02:28Z

src/DOMWrap.jsx

+      if (selectedIndex !== -1 && selectedIndex > lastVisibleIndex) {
+        const selectedIndexInOverflow = this.overflowedItems.findIndex(ele => ele.props.eventKey === this.childrenCache[selectedIndex].props.eventKey)
+        const tmp = this.props.children[lastVisibleIndex - 1];
+        this.props.children[lastVisibleIndex] = this.overflowedItems[selectedIndexInOverflow];


Might not be the best way since we're modifying this.props.children. Suggestions appreciated!

chenshuai2144 · 2018-06-19T15:16:22Z

src/DOMWrap.jsx

+  debouncedHandleResize = debounce(this.handleResize, 500);
+
+  componentDidMount() {
+    window.addEventListener('resize', this.debouncedHandleResize);


window.addEventListener('resize', this.resize, { passive: true });
加上这个稍微提升点性能

chenshuai2144 · 2018-06-19T15:20:15Z

src/DOMWrap.jsx

+            return childNode;
+          } else if (index === this.state.overflowingIndex) {
+            // put all the overflowed item inside a submenu with a title of overflow indicator ('...')
+            const copy = this.props.children[this.state.overflowingIndex];


overflowingIndex 用了好多次
搞个临时变量比较好

picodoth · 2018-06-25T11:45:32Z

@chenshuai2144 @yesmeck 再看下，加了test, 可以checkout出来感受下，看 examples/antd

coveralls · 2018-06-25T11:47:40Z

Coverage decreased (-0.2%) to 99.429% when pulling 9c20a5e on allow-overflow into 0efef0b on master.

chenshuai2144 · 2018-06-25T12:28:49Z

package.json

@@ -76,6 +76,7 @@
    "babel-runtime": "6.x",
    "classnames": "2.x",
    "dom-scroll-into-view": "1.x",
+    "lodash": "^4.17.10",


可以直接引用 lodash/debounce 包

chenshuai2144 · 2018-06-25T12:31:51Z

src/DOMWrap.jsx

+  renderChildren(children) {
+    // need to take care of overflowed items in horizontal mode
+    if (this.props.mode === 'horizontal') {
+      const lastVisibleIndex = this.state.lastVisibleIndex;


const { lastVisibleIndex} = this.state;

chenshuai2144 · 2018-06-25T12:32:52Z

src/DOMWrap.jsx

+    delete rest.visible;
+    delete rest.prefixCls;
+    delete rest.overflowedIndicator;
+    delete rest.mode;


为什么不直接在结构赋值就不要引入？

chenshuai2144 · 2018-06-25T12:32:54Z

src/DOMWrap.jsx

+    delete rest.visible;
+    delete rest.prefixCls;
+    delete rest.overflowedIndicator;
+    delete rest.mode;


为什么不直接在结构赋值就不要引入？

为什么不呢

afc163 · 2018-06-25T12:56:20Z

src/DOMWrap.jsx

@@ -1,7 +1,7 @@
 import React from 'react';
 import ReactDOM from 'react-dom';
 import PropTypes from 'prop-types';
-import debounce from 'lodash/debounce';
+import debounce from 'lodash.debounce';


Why change this?

https://www.npmjs.com/package/lodash.debounce

应该都可以？

picodoth · 2018-06-28T04:22:27Z

@yesmeck @afc163

yesmeck · 2018-06-30T06:18:10Z

package.json

@@ -76,6 +76,7 @@
    "babel-runtime": "6.x",
    "classnames": "2.x",
    "dom-scroll-into-view": "1.x",
+    "lodash.debounce": "^4.0.8",


统一用 lodash/debounce，这样不会跟其他包重复装依赖。

yesmeck · 2018-06-30T06:31:26Z

src/DOMWrap.jsx

+  };
+
+  componentDidMount() {
+    this.setChildrenCache();


children 变化了呢？

yesmeck · 2018-06-30T06:32:27Z

src/DOMWrap.jsx

+
+  componentDidMount() {
+    this.setChildrenCache();
+    this.setOverflowedIndicatorSize();


indicator 变化了呢？

yesmeck · 2018-06-30T06:34:01Z

src/DOMWrap.jsx

+  }
+
+  componentWillUnmount() {
+    window.removeEventListener('resize', this.debouncedHandleResize);


debounced 的方法最后一次调用要 cancel 掉。

this.debouncedHandleResize.cancel()

afc163 · 2018-07-02T03:52:18Z

src/DOMWrap.jsx

+      newState = {
+        ...newState,
+        children: nextProps.children,
+        childrenUpdated: true,


不应该用这个状态，应该在 children 发生变化的时候直接更新 childrenCache

这个状态是为了让组件有机会重新直接渲染 children, 见 https://github.com/react-component/menu/pull/159/files#diff-e2fa48f75c2dd2318295cde428556a76R239, 否则没有机会拿到正确的 childrenCahe

componentDidUpdate 里能直接比较 preProps.children === this.props.children。

@yesmeck 4d6640e

picodoth · 2018-07-04T06:27:55Z

src/DOMWrap.jsx

-        this.overflowedItems[selectedIndexInOverflow] = tmp;
+
+        this.overflowedItems.splice(selectedIndexInOverflow, 1);
+        this.overflowedItems.unshift(tmp);


picodoth · 2018-07-04T06:45:21Z

src/DOMWrap.jsx

@@ -185,18 +173,6 @@ class DOMWrap extends React.Component {

        lastVisibleIndex = lastVisibleIndex - 1;
      }
-
-      // try to hoist hidden selected item
-      if (selectedIndex !== -1 && selectedIndex > lastVisibleIndex) {


先 revert 了，选中项被隐藏的场景比较多比较复杂，如果有 feature request 再考虑加进来吧 @yesmeck @chenshuai2144

picodoth · 2018-07-11T04:17:07Z

src/DOMWrap.jsx

@@ -27,7 +28,7 @@ class DOMWrap extends React.Component {
  }

  componentDidUpdate(prevProps) {
-    if (prevProps.children !== this.props.children
+    if (menuChildrenChanged(prevProps.children, this.props.children)


@yesmeck 发现原来的版本在 antd 里每次hover children 都不一样，就会一致刷。

什么意思，组件从父组件 render 过来的话 children 就是每次都不一样的。

孙子节点有变化呢？

我改成了比较 key，假定 key 变了才认为 menuItem 改变

奇怪的是只是 hover 在 menuItem 上，为什么会触发父组件的 render

孙子节点的变化不影响顶级的渲染啊

没懂，所以你认为目前的方案有问题么

我是说孙子节点要是变化了，并不需要updateChildrenCache，因为不会影响第一层MenuItems。

class App extends React.Component { render() { return ( <Menu> <Menu.Item key="1"><Foo /><Menu.Item> </Menu> ); } }

App 重新 render，Foo 发生变化，这种情况，需不需要 updateChildrenCache

没意识到 MenuItem 的孩子才是真正的内容 Foo 发生变化咋比较好不能直接比较 reference 否则每次都不一样，不比较 reference 比较啥啊？

引用比较有什么问题？

每次 componentDidUpdate 都会重新 setState，在 antd 里 hover �在一个 popup 上会闪

picodoth · 2018-07-12T14:07:50Z

@chenshuai2144 @yesmeck ready to go!!!!

picodoth · 2018-07-13T02:48:39Z

FYI @afc163

yesmeck · 2018-07-13T03:10:54Z

自定义的会被遮住

picodoth · 2018-07-13T03:15:53Z

自定义的会被遮住

好的，edge case 有点多，这个我到时候再加进去也计算一下

你先整体再确认一下，看实现有啥问题

yesmeck · 2018-07-13T03:21:03Z

src/DOMWrap.jsx

+    const container = document.body.appendChild(document.createElement('div'));
+    container.setAttribute('style', 'position: absolute; top: 0; visibility: hidden');
+    ReactDOM.render(this.props.overflowedIndicator, container, () => {
+      this.overflowedIndicatorWidth = getWidth(container) + 40;


ReactDOM.unmountComponentAtNode(container)

yesmeck · 2018-07-13T03:21:18Z

src/DOMWrap.jsx

+        });
+
+        this.originalScrollWidth = scrollWidth;
+


ReactDOM.unmountComponentAtNode(container)

picodoth · 2018-07-13T04:24:09Z

@yesmeck 修复了简化了代码

fix leftover corner cases fixing selected keys issue fix selected menuItem got hidden scenario cleaning up fix lint fix scroll into view fix more item eventkey issue and cleanup addressing corner cases extract getWidth and getScrollWidth for test adding test update tests update readme adding more docs fix lint address cr handle children or overflowedIndicator changes fix lint use componentDidUpdate rather than getDerivedStateFromProps update debounce time and example fix uncontrolled mode fixing lint and adding text try to keep order while hoisting hidden selected items revert hoisting selected keys

picodoth · 2018-07-14T06:12:00Z

没啥问题，合了哈

This reverts commit c96400a.

picodoth requested review from yesmeck and chenshuai2144 June 19, 2018 14:57

picodoth force-pushed the allow-overflow branch from 5c2ec16 to bb3dd9d Compare June 19, 2018 14:58

picodoth commented Jun 19, 2018

View reviewed changes

chenshuai2144 reviewed Jun 19, 2018

View reviewed changes

picodoth force-pushed the allow-overflow branch 3 times, most recently from 39e584f to 7672518 Compare June 25, 2018 11:34

picodoth changed the title ~~[WIP] feat: allow overflow in horizontal mode~~ feat: allow overflow in horizontal mode Jun 25, 2018

picodoth force-pushed the allow-overflow branch 2 times, most recently from 701f3ad to 64ce8ba Compare June 25, 2018 11:44

picodoth force-pushed the allow-overflow branch from 64ce8ba to 867ba8f Compare June 25, 2018 11:46

chenshuai2144 reviewed Jun 25, 2018

View reviewed changes

afc163 reviewed Jun 25, 2018

View reviewed changes

picodoth force-pushed the allow-overflow branch 2 times, most recently from e028ba3 to 5e1c8aa Compare June 26, 2018 01:54

yesmeck reviewed Jun 30, 2018

View reviewed changes

src/DOMWrap.jsx Outdated

};

componentDidMount() {

this.setChildrenCache();

Copy link

Member

yesmeck Jun 30, 2018

Choose a reason for hiding this comment

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

children 变化了呢？

yesmeck reviewed Jun 30, 2018

View reviewed changes

afc163 reviewed Jul 2, 2018

View reviewed changes

picodoth force-pushed the allow-overflow branch 2 times, most recently from 2b53cbf to c46b2d1 Compare July 2, 2018 06:13

picodoth commented Jul 4, 2018

View reviewed changes

picodoth force-pushed the allow-overflow branch 2 times, most recently from e34879b to 949614f Compare July 4, 2018 07:23

picodoth mentioned this pull request Jul 11, 2018

feat: prevent horizontal menu items wrapping ant-design/ant-design#11234

Merged

picodoth force-pushed the allow-overflow branch from e7ce7d5 to 5921ab0 Compare July 11, 2018 04:15

picodoth commented Jul 11, 2018

View reviewed changes

picodoth force-pushed the allow-overflow branch from fd558a4 to ea14cb5 Compare July 12, 2018 14:05

picodoth force-pushed the allow-overflow branch from ea14cb5 to f819c10 Compare July 13, 2018 02:44

yesmeck reviewed Jul 13, 2018

View reviewed changes

src/DOMWrap.jsx Outdated

});

this.originalScrollWidth = scrollWidth;

Copy link

Member

yesmeck Jul 13, 2018

Choose a reason for hiding this comment

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

ReactDOM.unmountComponentAtNode(container)

picodoth force-pushed the allow-overflow branch from 5efd1d1 to cf12a08 Compare July 13, 2018 04:23

picodoth force-pushed the allow-overflow branch from cf12a08 to a2ee07d Compare July 13, 2018 06:03

picodoth added 2 commits July 14, 2018 14:00

fix children updating problem

e0a10f6

picodoth force-pushed the allow-overflow branch from a2ee07d to 346d014 Compare July 14, 2018 06:00

address cr

9c20a5e

picodoth force-pushed the allow-overflow branch from 346d014 to 9c20a5e Compare July 14, 2018 06:08

picodoth merged commit c66f7c1 into master Jul 14, 2018

picodoth deleted the allow-overflow branch July 14, 2018 06:12

yesmeck added a commit that referenced this pull request Jul 17, 2018

Revert #159

c96400a

picodoth added a commit that referenced this pull request Jul 18, 2018

Revert "Revert #159"

9b40591

This reverts commit c96400a.

picodoth added a commit that referenced this pull request Aug 19, 2018

Revert "Revert #159"

5c3913b

This reverts commit c96400a.

picodoth added a commit that referenced this pull request Aug 27, 2018

Revert "Revert #159"

b6921d3

This reverts commit c96400a.

feat: allow overflow in horizontal mode #159

feat: allow overflow in horizontal mode #159

Conversation

picodoth commented Jun 19, 2018 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

chenshuai2144 Jun 19, 2018 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

picodoth commented Jun 25, 2018 • edited

coveralls commented Jun 25, 2018 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

picodoth commented Jun 28, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

yesmeck Jul 11, 2018 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

picodoth Jul 11, 2018 • edited

Choose a reason for hiding this comment

picodoth commented Jul 12, 2018

picodoth commented Jul 13, 2018 • edited

yesmeck commented Jul 13, 2018

picodoth commented Jul 13, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

picodoth commented Jul 13, 2018

picodoth commented Jul 14, 2018

picodoth commented Jun 19, 2018 •

edited

chenshuai2144 Jun 19, 2018 •

edited

picodoth commented Jun 25, 2018 •

edited

coveralls commented Jun 25, 2018 •

edited

yesmeck Jul 11, 2018 •

edited

picodoth Jul 11, 2018 •

edited

picodoth commented Jul 13, 2018 •

edited