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

[WIP] add platform prop to menu #1200

Closed
wants to merge 1 commit into from

Conversation

cncolder
Copy link
Contributor

@cncolder cncolder commented Apr 22, 2017

Menu and SubMenu require List.Item.

This pr add platform prop to Menu and pass through to List.Item.

It seem like someone break lint. I commit this by git commit --no-verify.

BTW: The SSR has worked basically after this pr.

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.

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 @warmhug, @yiminghe and @pingan1927 to be potential reviewers.

@codecov
Copy link

codecov bot commented Apr 22, 2017

Codecov Report

Merging #1200 into master will not change coverage.
The diff coverage is 100%.

Impacted file tree graph

@@           Coverage Diff           @@
##           master    #1200   +/-   ##
=======================================
  Coverage   69.74%   69.74%           
=======================================
  Files         218      218           
  Lines        4125     4125           
  Branches     1218     1218           
=======================================
  Hits         2877     2877           
  Misses       1247     1247           
  Partials        1        1
Flag Coverage Δ
#rn 72.02% <ø> (ø) ⬆️
#web 67.37% <100%> (ø) ⬆️
Impacted Files Coverage Δ
components/menu/SubMenu.web.tsx 100% <100%> (ø) ⬆️
components/menu/index.web.tsx 91.3% <100%> (ø) ⬆️

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 3876389...04034ab. Read the comment docs.

@paranoidjk
Copy link
Contributor

menu 有这个 android 的水波纹效果是不是不太好?

@paranoidjk
Copy link
Contributor

CI lint 没问题,应该是你本地的依赖版本不同

@cncolder
Copy link
Contributor Author

@paranoidjk List.Item 的水波纹效果的实现方式有待商榷,默认情况下 platform='cross' 还会检测 navigator.userAgent,现阶段要想支持 SSR 就必须在服务端传入 platform。

@paranoidjk
Copy link
Contributor

@cncolder 这个问题是通用的,好几个组件都有 platform 这个api,https://github.com/ant-design/ant-design-mobile/search?l=TypeScript&q=platform&type=&utf8=%E2%9C%93 如果ssr的时候服务端指定 platform 其实也不太好指定哪个。

你看看你有没有意向重构一下,platform="cross"的时候把真正的渲染延迟到componentDidMount之后做,就像 #1153 这里

@cncolder
Copy link
Contributor Author

cncolder commented Apr 22, 2017

@paranoidjk 目前有3个组件:ListItem Modal Switch

我在想 ListItem 这个组件使用上比较普遍,同一个页面会出现很多次,多一次 render 对 ListItem 的影响应该是最大的。

@paranoidjk
Copy link
Contributor

@cncolder 看了下 ListItem 的实现其实也不太好,https://github.com/ant-design/ant-design-mobile/blob/master/components/list/ListItem.web.tsx#L131 这里可以去掉 isAndroid 的判断,改成默认display: none,那就不用在 render 里依赖 navigator 了

@cncolder
Copy link
Contributor Author

cncolder commented Apr 22, 2017

@paranoidjk 你是说在 onClick 里加一行

if (!isAndroid) coverRipleStyle.display = 'none';

或者这样行不行,删掉 render 里的 isAndroid,让 iOS 也渲染一条 <div style={coverRipleStyle} className={ripleCls} /> 貌似也没什么问题。

@paranoidjk
Copy link
Contributor

@cncolder 我的意思也是就始终渲染一个不可见的div,按这个方案做吧

@cncolder cncolder changed the title add platform prop to menu [WIP] add platform prop to menu Apr 26, 2017
@cncolder cncolder mentioned this pull request Apr 26, 2017
5 tasks
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.

3 participants