Skip to content

Conversation

@zombieJ
Copy link
Member

@zombieJ zombieJ commented May 30, 2018

  • Upgrade rc-tools & fix eslint
  • Remove dependency css-animation and use react lifecycle instead
  • Use raf for active state trigger
  • Align the transition/animation event with react
    • still use js event inject for keeping the origin behavior
  • Use new react-lifecycles-compat for new life cycle method
  • exclusive change can change the transition immediately
  • Add more test case

Currently, className will be added by both React & js to keep the className work when sub component not accept the className props. But for styles, there is no better way for updating.
So it still has the issue when both React and animation change the style prop.

@zombieJ zombieJ requested a review from yesmeck May 30, 2018 14:56
@afc163
Copy link
Member

afc163 commented May 30, 2018

wow~ what is the primary commit for fixing?

if (window.callPhantom) {
return done();
done();
return;
Copy link
Member

Choose a reason for hiding this comment

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

why change this?

Copy link
Member Author

Choose a reason for hiding this comment

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

eslint

@zombieJ
Copy link
Member Author

zombieJ commented May 30, 2018

Primary for React will reset className when component trigger re-render.
Since origin code use js for update the class of dom which will be override by React batch update in commitWork.

ref: #38

@zombieJ
Copy link
Member Author

zombieJ commented May 31, 2018

@afc163 @yesmeck ,
This will used for rc-trigger, could you help for code review.

Thanks

@@ -1,336 +0,0 @@
import React from 'react';
Copy link
Member

Choose a reason for hiding this comment

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

别重命名,diff 没法看了。

Copy link
Member Author

Choose a reason for hiding this comment

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

代码逻辑从 Animate keys 转移到 AnimateChild 里了, diff 也没参考价值...

@zombieJ
Copy link
Member Author

zombieJ commented Jun 1, 2018

Mark: rc-menu code using workaround for origin animate issue. Should adjust haveRendered logic: https://github.com/react-component/menu/blob/master/src/SubMenu.jsx#L370
@picodoth

@zombieJ
Copy link
Member Author

zombieJ commented Jun 1, 2018

Mark: collapse in antd add empty apear() {} for appear trigger. This could pass as null:
https://github.com/ant-design/ant-design/blob/master/components/collapse/Collapse.tsx#L25
@afc163

(Inline code use css-animation for style changing. Let's just keep it)

@zombieJ
Copy link
Member Author

zombieJ commented Jun 1, 2018

Mark: rc-tree using React.Children.map to create the clone TreeNode which changes the key when next render: https://github.com/react-component/tree/blob/master/src/Tree.jsx#L715

Update: Also affected by empty apear() {}. Set to null can fix this.

@zombieJ
Copy link
Member Author

zombieJ commented Jun 4, 2018

@afc163 @yesmeck , 验证了一圈应该就 menu collapsetree 需要调整一下,其他的动画都没有问题。 发一个 rc-aniamte 大版本,就能慢慢替掉了。

return Animate;
}

export default genAnimate(AnimateChild); No newline at end of file
Copy link
Member

Choose a reason for hiding this comment

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

为啥要套个 genAnimate 方法

Copy link
Member Author

Choose a reason for hiding this comment

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

用来生成 支持/不支持 Transition 和 Animation 的实例做 Test Coverage。

@jljsj33
Copy link
Member

jljsj33 commented Jun 11, 2018

哇操。。。。666

@zombieJ
Copy link
Member Author

zombieJ commented Jun 11, 2018

@afc163 @yesmeck, 看看有没有什么问题。
我准备发一版 3.0.0-rc 先走一圈看看对其他 rc 组件测试用例是否要更新。没问题就发 3.0.0 到 antd 3.7.0 里同步更新了。

@zombieJ zombieJ changed the base branch from master to 3.0.0 June 11, 2018 07:00
@zombieJ zombieJ merged commit 633d046 into react-component:3.0.0 Jun 11, 2018
@afc163
Copy link
Member

afc163 commented Jun 11, 2018

rc 组件估计也要改一轮依赖。

@zombieJ
Copy link
Member Author

zombieJ commented Jun 12, 2018

嗯,现在在调试 trigger,发了一版 rc-trigger@3.0.0-rc.1 会一个个换过来。

@zombieJ
Copy link
Member Author

zombieJ commented Jun 12, 2018

@zombieJ
Copy link
Member Author

zombieJ commented Jun 12, 2018

备注:
原本代码里关于 transition class 添加判断逻 isCssAnimationSupported || !props.animation[animationType],这里有点问题。后半段判断条件导致如果环境不支持 transition 也会添加 transition class 但是结束事件不会触发(getPropertyValue 拿到是空,后继被 css-animation 的 setTimeout 延迟 200ms 清理 )。
因而在测试环境里没有 transition-appear class 才是正确的。

新版逻辑复用了React的transition判断方式,对于不支持 transition 的环境(例如jest使用jsdom没有模拟transition),生命周期里便会处理掉。不过这会影响与之相关的所有测试用例(用到 rc-trigger 的)。

准备 tree-select 重构完后,把相关的其他组件的 test case 都更新一下。

return prefixes;
}

const vendorPrefixes = getVendorPrefixes(canUseDOM, window);
Copy link
Member

Choose a reason for hiding this comment

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

不能直接用 window,SSR 会报错。

Copy link
Member Author

Choose a reason for hiding this comment

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

好了

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.

4 participants