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

Make List.Item ssr friendly #1219

Merged
merged 5 commits into from
May 6, 2017
Merged

Conversation

cncolder
Copy link
Contributor

@cncolder cncolder commented Apr 26, 2017

Remove platform detect code from List.Item#render()

This pr make List.Item render riple div on all device. But still show ripple effect on android only.

close #1200

First of all, thanks for your contribution! :-)

Please makes sure these boxes are checked before submitting your PR, thank you!

  • Make sure you follow antd's code convention.
  • Run npm run lint and fix those errors before submitting in order to keep consistent code style.
  • Rebase before creating a PR to keep commit history clear.
  • Add some descriptions and refer relative issues for you PR.
  • Wait enough real device testing.

This change is Reviewable

@mention-bot
Copy link

@cncolder, thanks for your PR! By analyzing the history of the files in this pull request, we identified @yiminghe, @yesmeck and @pingan1927 to be potential reviewers.

@paranoidjk
Copy link
Contributor

测试ok吗? 至少要测试官方demo

@cncolder
Copy link
Contributor Author

@paranoidjk 我没有足够的设备,可以等等看,这个PR要慎重。

还有 riple 是拼错了吧,应该是 ripple。

@cncolder cncolder changed the title Make List.Item ssr friendly [WIP] Make List.Item ssr friendly (Until enough real device test) Apr 26, 2017
@paranoidjk
Copy link
Contributor

  • typo 可以改掉。
  • 这个看起来不需要兼容性测试,不需要多设备的,chrome模拟器看下就行。

注意这里有样式的
https://github.com/ant-design/ant-design-mobile/blob/master/components/list/style/index.less#L51

@codecov
Copy link

codecov bot commented Apr 26, 2017

Codecov Report

Merging #1219 into master will decrease coverage by 0.01%.
The diff coverage is 60%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #1219      +/-   ##
==========================================
- Coverage   61.94%   61.93%   -0.02%     
==========================================
  Files         219      219              
  Lines        4155     4153       -2     
  Branches     1225     1223       -2     
==========================================
- Hits         2574     2572       -2     
  Misses       1580     1580              
  Partials        1        1
Flag Coverage Δ
#rn 63.32% <ø> (ø) ⬆️
#web 60.5% <60%> (-0.04%) ⬇️
Impacted Files Coverage Δ
components/list/ListItem.web.tsx 69.64% <60%> (-1.05%) ⬇️

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 7e9dfd7...0cd6d7a. Read the comment docs.

@cncolder
Copy link
Contributor Author

demo 目测正常

@cncolder cncolder changed the title [WIP] Make List.Item ssr friendly (Until enough real device test) Make List.Item ssr friendly Apr 26, 2017
@@ -112,7 +112,6 @@ class ListItem extends React.Component<ListItemProps, any> {
[`${prefixCls}-arrow-vertical-up`]: arrow === 'up',
});

const isAndroid = platform === 'android' || (platform === 'cross' && !!navigator.userAgent.match(/Android/i));
Copy link
Contributor

Choose a reason for hiding this comment

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

核心的改动只是这一行吧?

Copy link
Contributor Author

@cncolder cncolder Apr 27, 2017

Choose a reason for hiding this comment

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

是的,render 阶段不再判断平台,避免 platform 默认为 cross 时引用 navigator。缺点是 iOS 会多渲染一行 div。

Copy link
Contributor

Choose a reason for hiding this comment

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

这行div默认应该是没样式,不占空间的把?有必要的话display:none掉

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@paranoidjk Chrome 里显示是 0x0

Copy link
Contributor

Choose a reason for hiding this comment

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

那就不行,要处理下

@cncolder cncolder force-pushed the ssr-listitem branch 2 times, most recently from 0b53eac to 3184617 Compare May 2, 2017 06:22
}, () => {
this.debounceTimeout = setTimeout(() => {
this.setState({
coverRipleStyle: {},
RipleClicked: false,
coverRippleStyle: { display: 'none' },
Copy link
Contributor Author

@cncolder cncolder May 2, 2017

Choose a reason for hiding this comment

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

@paranoidjk review again

Copy link
Contributor

Choose a reason for hiding this comment

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

没看到有设置display block的时候?

Copy link
Contributor Author

@cncolder cncolder May 4, 2017

Choose a reason for hiding this comment

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

@paranoidjk 因为没有必要,显示波浪效果的时候会替换 coverRippleStyle,这里是两种 State 之间的切换。

此处的 display: 'none' 不在 css 文件里。

Copy link
Contributor

Choose a reason for hiding this comment

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

代码在你的repo上,我拉下来跑比较麻烦。

react props.style 的替换难道不是 merge 而是整个 replace 的?之前设置了 display:none, render 之后这个 css 就会保留在 dom 树里,下一次 state 切换,props.style 里面没有显式的设置 display:block, 这也行?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@paranoidjk 不是这样吗?

如果是 merge,那波浪显示后为什么没有重置 width/height/left/top ?

<!-- 点击前 -->
<div class="am-list-ripple" style="display: none;"></div>
<!-- 波浪效果显示中 -->
<div class="am-list-ripple am-list-ripple-animate" style="width: 1064px; height: 1064px; left: 233px; top: -430px;"></div>
<!-- 一秒后 -->
<div class="am-list-ripple" style="display: none;"></div>

拉取 pr 可以用 git pr,比如我在本地拉取 bccsafe 的 pr#1262

☃  ant-design-mobile git:(master) git pr 1262 ant-design
remote: Counting objects: 44, done.
remote: Compressing objects: 100% (8/8), done.
remote: Total 44 (delta 33), reused 33 (delta 33), pack-reused 3
Unpacking objects: 100% (44/44), done.
From https://github.com/ant-design/ant-design-mobile
 * [new ref]           refs/pull/1262/head -> pr/1262
Switched to branch 'pr/1262'

☃  ant-design-mobile git:(pr/1262) git log -n 3

commit afe822a2540b38459fa9fc3bd2c599c437be9a09
Author: BccSafe <bccsafe5988@gmail.com>
Date:   Fri May 5 19:41:28 2017 +0800

    update snapshot

commit 813eae6867228d53c75e3fd11456b28f0b6e2524
Author: BccSafe <bccsafe5988@gmail.com>
Date:   Thu May 4 13:55:54 2017 +0800

    fix style tsx

commit ba7278a6e95fc81f1dcf85fd145bc697c0ffde2d
Author: slientcloud <rjmuqiang@gmail.com>
Date:   Fri May 5 13:29:33 2017 +0800

    feature: split pagination from rn-carousel, ref #1146 (#1268)

Copy link
Contributor

Choose a reason for hiding this comment

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

  • 是的,你是对的,style attribute 是 replace 的。
  • 我一般是 git checkout pr/1219

@paranoidjk paranoidjk merged commit 4e95093 into ant-design:master May 6, 2017
lixiaoyang1992 pushed a commit to lixiaoyang1992/ant-design-mobile that referenced this pull request Apr 26, 2018
* remove platform detect from list item render

* test: update snapshot, caused by list-item riple

* fix typo ripple

* test: update snapshot, caused by typo ripple

* hide inactive ripple div
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