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

Feature/sticky #1696

Open
wants to merge 14 commits into
base: main
Choose a base branch
from
Open

Feature/sticky #1696

wants to merge 14 commits into from

Conversation

JerryChan31
Copy link
Contributor

@JerryChan31 JerryChan31 commented Nov 28, 2021

@vercel
Copy link

vercel bot commented Nov 28, 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/293WUA432P6RCPwTbnZErpELd8Uj
✅ Preview: https://naive-ui-git-fork-jerrychan31-feature-sticky-tusimple.vercel.app

[Deployment for 72baa09 failed]

@codecov
Copy link

codecov bot commented Nov 28, 2021

Codecov Report

Merging #1696 (b00ee42) into main (9a539f4) will decrease coverage by 0.11%.
The diff coverage is 54.76%.

❗ Current head b00ee42 differs from pull request most recent head db1f138. Consider uploading reports for the commit db1f138 to get more accurate results
Impacted file tree graph

@@            Coverage Diff             @@
##             main    #1696      +/-   ##
==========================================
- Coverage   64.88%   64.76%   -0.12%     
==========================================
  Files         886      886              
  Lines       17198    17255      +57     
  Branches     4072     4089      +17     
==========================================
+ Hits        11159    11176      +17     
- Misses       5278     5307      +29     
- Partials      761      772      +11     
Impacted Files Coverage Δ
src/auto-complete/src/AutoComplete.tsx 55.45% <ø> (ø)
src/data-table/src/styles/index.cssr.ts 100.00% <ø> (ø)
src/data-table/src/use-table-data.ts 64.55% <0.00%> (ø)
src/dialog/src/styles/index.cssr.ts 100.00% <ø> (ø)
src/input/src/styles/input.cssr.ts 100.00% <ø> (ø)
src/popover/src/Popover.tsx 65.47% <ø> (+0.77%) ⬆️
src/popover/src/PopoverBody.tsx 73.75% <ø> (ø)
src/popover/src/styles/index.cssr.ts 100.00% <ø> (ø)
src/switch/src/styles/index.cssr.ts 100.00% <ø> (ø)
src/upload/index.ts 100.00% <ø> (ø)
... and 15 more

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 9a539f4...db1f138. Read the comment docs.

@07akioni
Copy link
Collaborator

这个 sticky 是否存在条件限制?比如说某些 props 组合下不生效之类的

@07akioni
Copy link
Collaborator

image

这块 border 有没有可能跟着下面的一起合并了,变成一条线

@JerryChan31
Copy link
Contributor Author

这块 border 有没有可能跟着下面的一起合并了,变成一条线

调整一下bordered的实现方式是可以的。要让外面这个边框在header的兄弟节点上渲染。现在是因为在header的祖先节点上渲染边框,所以叠不起来。

@JerryChan31
Copy link
Contributor Author

JerryChan31 commented Nov 29, 2021

这个 sticky 是否存在条件限制?比如说某些 props 组合下不生效之类的

这个我可以晚点详细测试一下。文档里的例子试一遍 sticky 是否生效是不是就可以了?


试了一下现在固定头部和列这个例子还有点问题,看起来是z-index的原因,我晚点再提交一下修改这个。

@JerryChan31
Copy link
Contributor Author

这块 border 有没有可能跟着下面的一起合并了,变成一条线

想到一个解决办法是,给table-header加一个-1pxmargin-bottom,就可以重叠了…

@07akioni
Copy link
Collaborator

07akioni commented Nov 29, 2021

这块 border 有没有可能跟着下面的一起合并了,变成一条线

想到一个解决办法是,给table-header加一个-1pxmargin-bottom,就可以重叠了…

这样在 border radius 比较大的时候应该不太行,可以再花时间考虑考虑,如果有可能是最好的

@JerryChan31
Copy link
Contributor Author

这块 border 有没有可能跟着下面的一起合并了,变成一条线

想到一个解决办法是,给table-header加一个-1pxmargin-bottom,就可以重叠了…

这样在 border radius 比较大的时候应该不太行,可以再花时间考虑考虑,如果有可能是最好的

clip-path应该可以做到。我再花时间想想~

@JerryChan31
Copy link
Contributor Author

2021-11-30.4.13.51.mov

@JerryChan31
Copy link
Contributor Author

JerryChan31 commented Dec 1, 2021

顺便 fix 了:#1712

@JerryChan31 JerryChan31 changed the title Feature/sticky WIP: Feature/sticky Dec 1, 2021
@JerryChan31 JerryChan31 changed the title WIP: Feature/sticky Feature/sticky Dec 1, 2021
@07akioni
Copy link
Collaborator

07akioni commented Dec 2, 2021

2021-11-30.4.13.51.mov

这效果感觉看着很🐮

@07akioni
Copy link
Collaborator

07akioni commented Dec 2, 2021

圆角有问题的那个 issue 感觉还没那么简单,需要考虑的 case 很多,我先把那个框子 dom 的 overflow 弄成 hidden 了,这样可以保证 work。

在这个提交里 b00ee42

我感觉可能会影响你的调整(sticky 的判定),如果确实产生问题的话就只能用比较复杂的情况来给每个需要 radius 的地方都加上了 radius 了(我试了试感觉有四五种 case,还不能光靠 css)

@JerryChan31
Copy link
Contributor Author

圆角有问题的那个 issue 感觉还没那么简单,需要考虑的 case 很多,我先把那个框子 dom 的 overflow 弄成 hidden 了,这样可以保证 work。

在这个提交里 b00ee42

我感觉可能会影响你的调整(sticky 的判定),如果确实产生问题的话就只能用比较复杂的情况来给每个需要 radius 的地方都加上了 radius 了(我试了试感觉有四五种 case,还不能光靠 css)

确实会影响,哈哈。

没关系,慢慢调整到满意为止就好~

可以大概说一下有哪些case需要考虑吗?再改的时候可以把这些考虑上。

@07akioni
Copy link
Collaborator

07akioni commented Dec 5, 2021

,哈哈。

没关系,慢慢调整到满意为止就好~

可以大概说

大概是这几点:

table 会根据不同的情况选择是否拆分 DOM,比如 auto layout 或者非 fix layout,fix header 或者 column 都会影响到 table DOM 结构

有的时候是切了上下,有的时候是一个整体但是放在 n-scrollbar 里面,有的时候是一个整体也不套 n-scrollbar。

这样的影响就是在不同的 case 下需要调整的 DOM 不一样,比如切了上下我们要对左上角,右上角,左下角右下角各自调整

放在 scrollbar 里面的时候感觉 sticky 就没有可能生效了 = =...

最后的 case 倒是简单点,但是触发的条件很苛刻

image

image

header 和 body 挂在一个 table 的前提是“非 flex height 并且没有 max height”

body 里面分可以滚和不可以滚,包滚动条的有“有 scrollX 或者有 maxHeight 或者 flexHeight“,以及“是一个最基本的 auto layout”,这种情况下就是虽然什么都没设定我也让 table 要能滚,这是给用户开个口子让最基本的 table 在溢出后可以滚(我也不知道这个好不好)。

这里面就涉及不同情况下 DOM 的调整了,感觉好像全都覆盖是有点苛刻

目前看来就是如果全都被放在滚动条里的话,sticky 应该非常难实现,因为这个需要 table overflow visible

实现可能性比较大的就是头和主体分离的(但是这种情况如果要处理 border radius 需要精确到单元格的 border radius 定位,还要考虑跨行列的情况,肯定要在 JS 层计算出来然后做一些 class 的标记),或者没有被包上滚动条的

@vercel
Copy link

vercel bot commented Jan 21, 2022

@JerryChan31 is attempting to deploy a commit to the Tusimple Team on Vercel.

A member of the Team first needs to authorize it.

@JerryChan31
Copy link
Contributor Author

之前有点忙,搁置了一下这个PR,现在闲下来打算继续搞,eslint好像遇到了点问题,本地npm run lint:fix没有报错,但是在ci里会报错……我先研究一下怎么恢复到跟主干分支一样…

@07akioni
Copy link
Collaborator

之前有点忙,搁置了一下这个PR,现在闲下来打算继续搞,eslint好像遇到了点问题,本地npm run lint:fix没有报错,但是在ci里会报错……我先研究一下怎么恢复到跟主干分支一样…

rebase 一下就行

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