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-v2] header rendering is misplaced #18628

Merged
merged 4 commits into from
Oct 22, 2024

Conversation

btea
Copy link
Member

@btea btea commented Oct 22, 2024

Please make sure these boxes are checked before submitting your PR, thank you!

  • 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.

fix #18557

Copy link

github-actions bot commented Oct 22, 2024

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

pkg-pr-new bot commented Oct 22, 2024

Open in Stackblitz

pnpm add https://pkg.pr.new/element-plus/element-plus@18628

commit: d17e562

Copy link

github-actions bot commented Oct 22, 2024

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

@ShetePro
Copy link

image image Wouldn't it be better to get ScrollLeft in Table-Grid and pass it to Header?

Copy link
Collaborator

@warmthsea warmthsea left a comment

Choose a reason for hiding this comment

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

It seems that there is redundant execution. Would it be better to add a condition?

_left && scrollToLeft(_left)

screenshots

@btea
Copy link
Member Author

btea commented Oct 22, 2024

Wouldn't it be better to get ScrollLeft in Table-Grid and pass it to Header?

Thank you for your suggestion, this is indeed a more reasonable approach.

@btea
Copy link
Member Author

btea commented Oct 22, 2024

It seems that there is redundant execution. Would it be better to add a condition?

Yeah, it has been adjusted.

@warmthsea
Copy link
Collaborator

On the contrary, I think the 07f5d25 method is the best, because now onUpate is executed too many times and many useless operations are performed. (Personal feeling)

screenshots

@ShetePro
Copy link

On the contrary, I think the 07f5d25 method is the best, because now onUpate is executed too many times and many useless operations are performed. (Personal feeling)

Yeah, I tried both methods, and onUpdate still gets triggered unnecessarily during scrolling. However, I believe that directly extracting value from props is better than accessing DOM variables multiple times.

Additionally, if I want to reduce the number of calls, I think it can be done like this:

image

@btea
Copy link
Member Author

btea commented Oct 22, 2024

On the contrary, I think the 07f5d25 method is the best, because now onUpate is executed too many times and many useless operations are performed. (Personal feeling)

Good Catch 👍

Copy link
Collaborator

@warmthsea warmthsea left a comment

Choose a reason for hiding this comment

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

LGTM

@warmthsea warmthsea merged commit 1c1b274 into dev Oct 22, 2024
12 checks passed
@warmthsea warmthsea deleted the fix/table-v2-header-misplacement branch October 22, 2024 16:17
@element-bot element-bot mentioned this pull request Nov 1, 2024
3 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
CommitMessage::Qualified Qualified commit message
Projects
None yet
3 participants