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

fix(components): [table] row has rowspan when hover it only has background on the first row of this rowspan #15529

Merged
merged 22 commits into from Feb 4, 2024

Conversation

dowinweb
Copy link
Contributor

@dowinweb dowinweb commented Jan 16, 2024

table row has rowspan when hover it only has background on the first row of this rowspan
the other rows of which share the rowspan should also give the background of this cell when hover the other rows
Please make sure these boxes are checked before submitting your PR, thank you!

fix #15384

  • Make sure you follow contributing guide English | (中文 | Español | Français).
  • Make sure you are merging your commits to dev branch.
  • Add some descriptions and refer to relative issues for your PR.

table row has rowspan when hover it only has background on the first row of this colspan
Copy link

👋 @dowinweb, seems like this is your first time contribution to element-plus.

  • Please make sure that you have read our guidelines and code of conduct before making a contribution.
  • You can comment with /label Components:[component_name] to add a label for which component you are working on.
  • You may join our Discord for staying tuned.

@pull-request-triage pull-request-triage bot added 1st contribution Their very first contribution Needs Review labels Jan 16, 2024
Copy link

github-actions bot commented Jan 16, 2024

Copy link

Hello @dowinweb, thank you for contributing to element-plus, please see our guideline to see how to make contribution

@dowinweb
Copy link
Contributor Author

dowinweb commented Jan 16, 2024

fixed [Style] [table] el-table表合并之后,鼠标移动上去样式错误 #15384
image

@dowinweb dowinweb changed the title fix(components): table fix(components): table [row has rowspan when hover it only has background on the first row of this colspan] Jan 16, 2024
@dowinweb dowinweb changed the title fix(components): table [row has rowspan when hover it only has background on the first row of this colspan] fix(components): table [row has rowspan when hover it only has background on the first row of this rowspan] Jan 16, 2024
@dowinweb
Copy link
Contributor Author

dowinweb commented Jan 16, 2024

need the ID column also has the background of the hover row
image

@dowinweb dowinweb changed the title fix(components): table [row has rowspan when hover it only has background on the first row of this rowspan] fix(components): [table] row has rowspan when hover it only has background on the first row of this rowspan Jan 16, 2024
@dowinweb
Copy link
Contributor Author

@btea 大佬麻烦帮看下 项目table hover时候有问题 急需修复下

@github-actions github-actions bot added the CommitMessage::Qualified Qualified commit message label Jan 17, 2024
Copy link

github-actions bot commented Jan 17, 2024

🧪 Playground Preview: https://element-plus.run/?pr=15529
Please comment the example via this playground if needed.

@btea
Copy link
Collaborator

btea commented Jan 17, 2024

cc @element-plus/backers

@dowinweb
Copy link
Contributor Author

cc @kooriookami

@dowinweb
Copy link
Contributor Author

example

@FrontEndDog
Copy link
Member

感谢你的贡献,我认为鼠标悬浮在合并的单元格的时候,应该将合并的单元格所在的行都填充背景色,这样更符合直觉一些。

Thank you for your contribution, I think when hovering over merged cells, it is more intuitive to fill the rows of merged cells with background color.

fixed when hover on a rowspan > 1 cell give the whole rows background
@dowinweb
Copy link
Contributor Author

dowinweb commented Jan 18, 2024

感谢你的贡献,我认为鼠标悬浮在合并的单元格的时候,应该将合并的单元格所在的行都填充背景色,这样更符合直觉一些。

Thank you for your contribution, I think when hovering over merged cells, it is more intuitive to fill the rows of merged cells with background color.

You are right. I have fixed this.

@dowinweb
Copy link
Contributor Author

dowinweb commented Jan 18, 2024

@FrontEndDog At first i finish this by writing code in the watch hoverRow method, but when you move your mouse from the first row of the merged cell rows, to the merged cell who has rowSan > 1, it didnot trigger the watch method, because the hoverRow didnot change. so i have to do this in the cell event listeners.
image

@dowinweb
Copy link
Contributor Author

@FrontEndDog bro, can this be merged ? any more suggestions?

dwaynewdong and others added 2 commits January 20, 2024 14:41
fixed hover on merged cell give the background of the whole rows
import addClass removeClass
@dowinweb
Copy link
Contributor Author

@btea anything else need to fix

add test case for hover on rowspan > 2
@dowinweb
Copy link
Contributor Author

dowinweb commented Jan 22, 2024

如果可以加一下相关的测试用例,就更好了

It would be even better if you could add a test case!

OK already added test case
@FrontEndDog take a look if its ok

@dowinweb
Copy link
Contributor Author

tell me if there is anything else to change @btea @FrontEndDog

dwaynewdong and others added 4 commits January 23, 2024 22:17
use getTestData is better
use getTestData instead
change templete label prop
Copy link
Member

@FrontEndDog FrontEndDog left a comment

Choose a reason for hiding this comment

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

Thank you for your contribution!

@dowinweb
Copy link
Contributor Author

dowinweb commented Jan 31, 2024

@btea can this be merged? 😃 need to fix any other things?🤨

@dowinweb dowinweb requested a review from btea February 1, 2024 02:16
dwaynewdong added 2 commits February 1, 2024 10:54
@btea btea requested a review from tolking February 1, 2024 05:38
@tolking
Copy link
Member

tolking commented Feb 1, 2024

当表格包含固定列同时 rowspan>2 时,样式会出错

鼠标在 ID 的列上下移动 BUG

when there is fixed row, hover on rowSpan > 1 should not clear the class
@dowinweb
Copy link
Contributor Author

dowinweb commented Feb 1, 2024

当表格包含固定列同时 rowspan>2 时,样式会出错

鼠标在 ID 的列上下移动 BUG

不得不说行固定的时候 执行的有点奇怪,我去标记了固定时候的行,以避免被清空行样式,我觉得修复好了 可以测试下
I have to say that row with fixed works a little bit strange with the code process , i have to mark the row with fixed class to avoid the remove of the row background class, i think this is fixed, you can test it.

@tolking see if there is other conditions and boundary situations to test

@dowinweb
Copy link
Contributor Author

dowinweb commented Feb 4, 2024

@tolking what do i need to do now

Copy link
Member

@tolking tolking left a comment

Choose a reason for hiding this comment

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

LGTM.

Thank for your contribution

@tolking tolking merged commit d2fbff7 into element-plus:dev Feb 4, 2024
8 checks passed
@element-bot element-bot mentioned this pull request Feb 18, 2024
3 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[Style] [table] el-table表合并之后,鼠标移动上去样式错误
4 participants