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

Layout.Sider support responsive display #4931

Merged
merged 4 commits into from
Mar 5, 2017

Conversation

ddcat1115
Copy link
Contributor

  • only support one breakpoint now
  • with default style

normally
2017-02-18 1 59 30

below the breakpoint
2017-02-18 1 58 25

below the breakpoint and expanded
2017-02-18 1 58 15

@mention-bot
Copy link

@ddcat1115, thanks for your PR! By analyzing the history of the files in this pull request, we identified @afc163, @simaQ and @valentinvichnal to be potential reviewers.

@coveralls
Copy link

Coverage Status

Coverage increased (+0.01%) to 82.405% when pulling e001dcef917c9264f735c0917c117cf1edca4e31 on ddcat1115:fix-4668 into 63476d0 on ant-design:feature-2.8.

@afc163
Copy link
Member

afc163 commented Feb 17, 2017

It is 2:11, people should care their health...

@ddcat1115
Copy link
Contributor Author

Is that a message sent by a robot? 😹

@@ -3,6 +3,14 @@ import classNames from 'classnames';
import omit from 'omit.js';
import Icon from '../icon';

const dimensionMap = {
Copy link
Contributor

Choose a reason for hiding this comment

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

感觉我们需要考虑下 LESS 和 JS 共享变量的问题?如 https://www.npmjs.com/package/less-vars-loader

虽然不是很严重的问题,不排除会出现这里的 breakpoints 和 LESS 里面的不一样。

或者在两边都加上注释。

@@ -56,5 +56,8 @@ title: Layout
| trigger | 自定义 trigger,设置为 null 时隐藏 trigger | string\|ReactNode | - |
| width | 宽度 | number\|string | 200 |
| collapsedWidth | 收缩宽度,仅当 `collapsible:true` 时生效 | number | 64 |
| breakpoint | 触发响应式布局的断点 | string: `xs` \| `sm` \| `md` \| `lg` \| `xl` | - |
Copy link
Contributor

Choose a reason for hiding this comment

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

Enum { xs, sm, md, lg, xl }?

@@ -51,6 +64,24 @@ export default class Sider extends React.Component<SiderProps, any> {
}
}

componentDidMount() {
const matchMedia = window.matchMedia;
Copy link
Contributor

Choose a reason for hiding this comment

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

不用 polyfill 的原因是 Layout 强依赖于 flex,而支持 flex 的浏览器都支持 matchMedia

需要注释么?

Copy link
Member

@afc163 afc163 Feb 21, 2017

Choose a reason for hiding this comment

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

最好还是类似 Crasoul 加一下兼容,省得报错。

css 不支持 flex,页面也是不会报错的。

const props = this.props;
if (matchMedia && props.breakpoint && props.breakpoint in dimensionMap) {
const mql = matchMedia(`(max-width: ${dimensionMap[props.breakpoint]})`);
mql.addListener(this.responsiveHandler);
Copy link
Contributor

Choose a reason for hiding this comment

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

unmount 时需要取消监听?

@afc163
Copy link
Member

afc163 commented Feb 20, 2017

Conflict

belowTrigger = (
<span onClick={this.belowShowChange} className={`${prefixCls}-below-default-trigger`}>
<Icon type="bars" />
</span>);
Copy link
Member

Choose a reason for hiding this comment

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

这玩意用户可以去掉么?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

设置了 widthBelow(不为零) 或 collapsible 时都不会展现这个东西,仅当侧边宽度为 0 时展现。目前没有支持 belowTrigger 的自定义。

Copy link
Member

Choose a reason for hiding this comment

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

belowTrigger 和 trigger 合并如何?这两个是同一个东西。

Copy link
Member

Choose a reason for hiding this comment

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

当 collapsible 为 true 时,且屏幕宽度小于 breakPoint 时,菜单会自动缩小并触发 onCollapsed。

当 collapsedWidth 为 0 时,trigger 默认为当前的 belowTrigger 。

Copy link
Member

Choose a reason for hiding this comment

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

onResponse 的参数改为 (collapsed)

Copy link
Member

Choose a reason for hiding this comment

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

这样可以尽可能复用原来的属性。

Copy link
Contributor Author

Choose a reason for hiding this comment

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

这样做的话有几个问题:

  1. 如果希望小于 breakPoint 时完全隐藏 sider,就只能把 collapsedWidth 设置为零,那大于 breakPoint 且状态为收起时 sider 宽度也为零了。
  2. 如果复用原来的 collapse 状态,可能会混淆人为的操作(设定)与屏幕宽度变化带来的改变,比如:大于 breakpoint 时人为设置为 collapsed=true,此时缩小屏幕至 breakpoint 以下,再拉大屏幕至 breakpoint 以上,这时要不要展开?所以我觉得这里 collapsed 不应该被响应式影响

Copy link
Contributor Author

Choose a reason for hiding this comment

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

之前参考 gitlab 的方式,它的响应式反馈也不会影响 collapsed 状态,但小于 breakpoint 时 collapsible=false,觉得这种处理放在我们这有点太暴力,所以保留了两套体系

Copy link
Member

@afc163 afc163 Feb 24, 2017

Choose a reason for hiding this comment

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

  1. 问题不大,菜单只有两种状态,收起或展开,无论是手动收起还是小屏幕自动收起,展现都一样。收起的宽度也是固定的,要么一个小值,要么为 0(为 0 时有一个特殊的样式,就是悬浮的 bar)。参考 gitlab 的菜单。
  2. 同样。人为的操作和屏幕宽度变化带来的改变是一样的。如果 collapsed={true} 则始终展开,任何情况都不能收起。onCollapsedChange 响应两种变化。这样 onResponsive,也不需要了,直接作为一个参数 onCollapsedChange(collapsed, way: 'clickTrigger' | 'reponsive')

@ddcat1115
Copy link
Contributor Author

除了 @benjycui 提到的 js less 变量共享的问题外,其他已 fix

@coveralls
Copy link

Coverage Status

Coverage increased (+0.01%) to 82.312% when pulling 1e09011d6b620131fb2363f9f766748c41d9e4c5 on ddcat1115:fix-4668 into 6d903a6 on ant-design:feature-2.8.

@coveralls
Copy link

Coverage Status

Coverage increased (+0.01%) to 82.312% when pulling 1e09011d6b620131fb2363f9f766748c41d9e4c5 on ddcat1115:fix-4668 into 6d903a6 on ant-design:feature-2.8.

@benjycui
Copy link
Contributor

CI failed, 共享变量等这个 PR 合并后我再试吧。

@coveralls
Copy link

Coverage Status

Coverage increased (+0.01%) to 82.312% when pulling c67663dc06881b8a0c57b080fa178e8785abc2e5 on ddcat1115:fix-4668 into 6d903a6 on ant-design:feature-2.8.

@ddcat1115
Copy link
Contributor Author

CI 修复了

@@ -56,5 +56,8 @@ title: Layout
| trigger | 自定义 trigger,设置为 null 时隐藏 trigger | string\|ReactNode | - |
| width | 宽度 | number\|string | 200 |
| collapsedWidth | 收缩宽度,仅当 `collapsible:true` 时生效 | number | 64 |
| breakpoint | 触发响应式布局的断点 | Enum { 'xs', 'sm', 'md', 'lg', 'xl' } | - |
| widthBelow | 视窗宽度低于 breakpoint 时的 Sider 宽度 | number\|string | 默认为 0,即全部隐藏,当可收起时默认为 collapsedWidth |
Copy link
Member

@afc163 afc163 Feb 24, 2017

Choose a reason for hiding this comment

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

这个参数的应用场景是?直接用 collapsedWidth 如何?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

上面有提到原因,主要还是为了支持更多需求。不配置 widthBelow 且 collapsible=true 时默认就等于 collapsedWidth

@@ -55,5 +55,8 @@ onCollapse | the callback function, can be executed when you switch the sidebar,
trigger | specify the customized trigger, set to null to hide the trigger | string\|ReactNode| - |
width | width of the sidebar | number\|string | 200
collapsedWidth | width of the collapsed sidebar, available only `collapsible: true` | number | 64
breakpoint | breakpoint of the responsive layout | Enum { 'xs', 'sm', 'md', 'lg', 'xl' } | - |
Copy link
Member

Choose a reason for hiding this comment

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

breakpoint => breakPoint

然后是不是应该有默认值。

Copy link
Contributor Author

Choose a reason for hiding this comment

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

默认就要有响应式反馈吗?

Copy link
Member

Choose a reason for hiding this comment

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

我觉得可以,可以直接对小屏幕的布局进行默认优化。

Copy link
Contributor Author

Choose a reason for hiding this comment

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

还是没有做默认响应,因为宽度收缩需要配合侧边样式定制,如果默认加上可能导致折叠后样式不对

@yesmeck
Copy link
Member

yesmeck commented Mar 2, 2017

Need rebase

@coveralls
Copy link

Coverage Status

Coverage increased (+0.01%) to 81.715% when pulling 06713d6cc0a0078a0260f4a93b4651b429ad6620 on ddcat1115:fix-4668 into 0edb821 on ant-design:feature-2.8.

@coveralls
Copy link

Coverage Status

Coverage increased (+0.01%) to 81.715% when pulling 06713d6cc0a0078a0260f4a93b4651b429ad6620 on ddcat1115:fix-4668 into 0edb821 on ant-design:feature-2.8.

| trigger | 自定义 trigger,设置为 null 时隐藏 trigger | string\|ReactNode | - |
| width | 宽度 | number\|string | 200 |
| collapsedWidth | 收缩宽度,仅当 `collapsible:true` 时生效 | number | 64 |
| collapsedWidth | 收缩宽度,设置为 0 会出现特殊 trigger | number | 64 |
| breakPoint | 触发响应式布局的断点 | Enum { 'xs', 'sm', 'md', 'lg', 'xl' } | - |
Copy link
Member

Choose a reason for hiding this comment

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

Copy link
Member

Choose a reason for hiding this comment

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

Ok...it is my wrong suggestion. @ddcat1115

@coveralls
Copy link

Coverage Status

Coverage increased (+0.01%) to 81.907% when pulling a2f440d on ddcat1115:fix-4668 into 5af5a48 on ant-design:feature-2.8.

@coveralls
Copy link

Coverage Status

Coverage increased (+0.01%) to 81.907% when pulling a2f440d on ddcat1115:fix-4668 into 5af5a48 on ant-design:feature-2.8.

@yesmeck yesmeck merged commit 29fa65f into ant-design:feature-2.8 Mar 5, 2017
@thearabbit
Copy link

Problem with breadcrumb place, and I think that should be place the logo

image

@benjycui
Copy link
Contributor

@thearabbit you can try to over the trigger's style:

image

@benjycui
Copy link
Contributor

Add something like margin-top, so that it won't overlap your breadcrumb.

@thearabbit
Copy link

thearabbit commented Mar 23, 2017

Thanks for your reply.
Could I use it with Custom trigger.
It mean that by default I use custom trigger and when meet small device it is change to responsive auto like AdminLTE Bootstrap.
Please example 👍

@benjycui
Copy link
Contributor

@thearabbit try to set Layout.Sider[trigger=null]

@thearabbit
Copy link

Don't work.
Please help.

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.

None yet

7 participants