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

feat(layout): sider-position #657

Merged
merged 13 commits into from
Aug 7, 2021
Merged

feat(layout): sider-position #657

merged 13 commits into from
Aug 7, 2021

Conversation

bljessica
Copy link
Contributor

No description provided.

@vercel
Copy link

vercel bot commented Jul 26, 2021

This pull request is being automatically deployed with Vercel (learn more).
To see the status of your deployment, click below or on the icon next to each commit.

🔍 Inspect: https://vercel.com/tusimple/naive-ui/Da9jx2CpQe4QmetaSwFVePXMAo8C
✅ Preview: https://naive-ui-git-fork-bljessica-dev1-tusimple.vercel.app

@codecov
Copy link

codecov bot commented Jul 26, 2021

Codecov Report

Merging #657 (c2b5858) into main (3d5daa5) will decrease coverage by 0.02%.
The diff coverage is 18.18%.

Impacted file tree graph

@@            Coverage Diff             @@
##             main     #657      +/-   ##
==========================================
- Coverage   40.38%   40.36%   -0.03%     
==========================================
  Files         507      507              
  Lines       12398    12407       +9     
  Branches     3486     3490       +4     
==========================================
+ Hits         5007     5008       +1     
- Misses       6478     6485       +7     
- Partials      913      914       +1     
Impacted Files Coverage Δ
src/layout/src/LayoutSider.tsx 1.44% <0.00%> (-0.17%) ⬇️
src/layout/src/Layout.tsx 53.12% <66.66%> (-0.21%) ⬇️

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 3d5daa5...c2b5858. Read the comment docs.

Copy link
Collaborator

@07akioni 07akioni left a comment

Choose a reason for hiding this comment

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

写个 Demo 吧,带宽度折叠的那种

@@ -34,6 +34,7 @@ scroll-to
| has-sider | `boolean` | `false` | Whether the component has sider inside. If so it must be `true`. |
| native-scrollbar | `boolean` | `true` | Whether to use native scrollbar on itself. If set to `false`, layout will use a naive-ui style scrollbar for content. |
| position | `'static' \| 'absolute'` | `'static'` | `static` position will make it css position set to `static`. `absolute` position will make it css position set to `absolute` and `left`, `right`, `top`, `bottom` to `0`. `absolute` position is very useful when you want to make content scroll in a fixed container or make the whole page's layout in a fixed position. You may need to change the style of the component to make it display as you expect. |
| sider-positioned | `boolean` | `false` | Whether the component content is reversed, if you need to set the sidebar on the right, you must set it to `true`. |
Copy link
Collaborator

Choose a reason for hiding this comment

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

use sider-placement instead

@@ -66,6 +67,7 @@ scroll-to
| position | `'static' \| 'absolute'` | `'static'` | `static` 模式将会把 CSS `position` 设为 `static`, `absolute` 模式将会把 CSS `position` 设为 `absolute`,还将 `left`、`top`、`bottom` 设为 `0`。`absolute` 模式在你想将内容在一个固定容器或者将这个页面的布局设为固定位置的时候很有用。你可能需要修改一些 style 来确保它按照你预想的方式展示 |
| show-collapsed-content | `boolean` | `true` | 是否在 `sider` 折叠后展示内部内容 |
| show-trigger | `boolean \| 'bar' \| 'arrow-circle'` | `false` | 内置的触发按钮是否展示 |
| sider-position | `'left' \| 'right'` | `left` | 侧边栏显示在左侧还是右侧 |
Copy link
Collaborator

Choose a reason for hiding this comment

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

footer 需要这个吗?

const mergedTriggerStyle = computed(() => [
props.triggerStyle,
{
left: props.siderPosition === 'left' ? 'unset' : 0,
Copy link
Collaborator

Choose a reason for hiding this comment

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

props.siderPosition === 'left' 这个提一层,siderOnLeft = ...

return {
borderRight:
props.bordered && props.siderPosition === 'left'
? '1px solid var(--border-color)'
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
? '1px solid var(--border-color)'
? '1px solid var(--border-color)'

这块在 css 用类处理

Copy link
Collaborator

@07akioni 07akioni left a comment

Choose a reason for hiding this comment

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

现在的改动会让 trigger button 出问题,看看

@bljessica
Copy link
Contributor Author

现在的改动会让 trigger button 出问题,看看
@07akioni 啥问题呀?

@07akioni
Copy link
Collaborator

现在的改动会让 trigger button 出问题,看看
@07akioni 啥问题呀?

就是位置不对,和线上的版本不一致,你可以看 vercel 的 preview

@07akioni
Copy link
Collaborator

image

@07akioni
Copy link
Collaborator

位置错了
image

翻转的动画没了
image

@07akioni
Copy link
Collaborator

布局的样式出问题了。

压缩了内容
image

正确的:
image

@07akioni
Copy link
Collaborator

位置错了
image

翻转的动画没了
image

这个评论里提到的问题还在

@07akioni
Copy link
Collaborator

可以和线上的文档网站对比一下

Comment on lines 126 to 147
const siderPlacement = ref<'left' | 'right'>('left')
const siderOnLeft = computed(() => {
return siderPlacement.value === 'left'
})
const mergedToggleButtonStyle = computed(() => {
const translateXAndY = siderOnLeft.value
? 'translateX(50%) translateY(-50%)'
: 'translateX(-50%) translateY(-50%)'
const translateZ = siderOnLeft.value
? mergedCollapsedRef.value
? 'rotate(180deg)'
: 'rotate(0)'
: mergedCollapsedRef.value
? 'rotate(0)'
: 'rotate(180deg)'
return [
props.triggerStyle,
!siderOnLeft.value && { left: 0 },
{
transform: `${translateXAndY} ${translateZ}`
}
]
Copy link
Collaborator

Choose a reason for hiding this comment

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

我的直觉这个功能不至于写这么多样式在组件里,能简化一下吗😂

我单 review 已经看不出是不是有问题了。

能不能试着尽量把样式集中在 css 里,用类名解决?

Copy link
Collaborator

Choose a reason for hiding this comment

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

那样代码应该会少很多。

Comment on lines 126 to 129
const siderPlacement = ref<'left' | 'right'>('left')
const siderOnLeft = computed(() => {
return siderPlacement.value === 'left'
})
Copy link
Collaborator

Choose a reason for hiding this comment

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

这块用 inject,不要在 render 里面用 $parent 修改这个值

Comment on lines 238 to 243
const siderScrollContainerTransformStyle =
!this.siderOnLeft && this.collapseMode === 'transform'
? `translateX(${
parseInt(formatLength(this.width)) - parseInt(this.styleMaxWidth)
}px)`
: 'unset'
Copy link
Collaborator

Choose a reason for hiding this comment

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

这个能放到 css 里吗?用类名区分就可以

Copy link
Contributor Author

Choose a reason for hiding this comment

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

这个translateX的值要计算,怎么放到css里呀?

Comment on lines 313 to 314
!this.siderOnLeft &&
`${mergedClsPrefix}-layout-toggle-button--right`,
Copy link
Collaborator

Choose a reason for hiding this comment

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

这块可以不用这个类,它是 layout-sider DOM 的 children,用 css 就行了

Comment on lines 324 to 327
class={[
!this.siderOnLeft &&
`${mergedClsPrefix}-layout-toggle-bar--right`
]}
Copy link
Collaborator

Choose a reason for hiding this comment

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

这里也同理

Comment on lines 140 to 142
const siderOnLeft = computed(() => {
return siderPlacement === 'left'
})
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
const siderOnLeft = computed(() => {
return siderPlacement === 'left'
})
const siderOnLeftRef = computed(() => {
return siderPlacement === 'left'
})

return {}
}
)
const siderPlacement = layoutProps?.siderPlacement || 'left'
Copy link
Collaborator

Choose a reason for hiding this comment

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

不能这么写,会丢失响应式,直接在其他 computed 里面用就行

Comment on lines 128 to 135
if (!siderOnLeft.value && props.collapseMode === 'transform') {
return {
transform: `translateX(${
parseInt(formatLength(props.width)) -
parseInt(styleMaxWidthRef.value)
}px)`
}
}
Copy link
Collaborator

Choose a reason for hiding this comment

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

这个状态估计是不能支持百分比的宽度,在左侧时候是能支持的

`${mergedClsPrefix}-layout-sider--${this.siderPlacement}-positioned`,
this.bordered &&
`${mergedClsPrefix}-layout-sider--border-${
this.siderOnLeft ? 'right' : 'left'
Copy link
Collaborator

Choose a reason for hiding this comment

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

siderPlacement 就行吧?

Comment on lines 280 to 282
{
transition: 'transform .3s var(--bezier)'
}
Copy link
Collaborator

Choose a reason for hiding this comment

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

这个地方可以用

cB('layout-sider', [
  c('>', [
    cB('scrollbar', 'transition: transform .3s var(--bezier);')
  ])
])

来解决

src/layout/src/LayoutSider.tsx Outdated Show resolved Hide resolved
src/layout/src/LayoutSider.tsx Outdated Show resolved Hide resolved
src/layout/src/LayoutSider.tsx Outdated Show resolved Hide resolved
Comment on lines +130 to +132
transform: `translateX(calc(${formatLength(props.width)} - ${
styleMaxWidthRef.value
}px)`
Copy link
Collaborator

Choose a reason for hiding this comment

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

calc 在 transform 里用其实没啥用,不会用外层的值

@07akioni
Copy link
Collaborator

07akioni commented Aug 7, 2021

做了点修改,去掉了一些没用的代码

3f8e640

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.

折叠侧边栏能否可以设置在右边
2 participants