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: update freeze incorrect when insert row #2492

Merged
merged 3 commits into from
Jun 19, 2024

Conversation

semmywong
Copy link
Contributor

close #2488

Pull Request Checklist

  • Related tickets or issues have been linked in the PR description (or missing issue).
  • Naming convention is followed (do please check it especially when you created new plugins, commands and resources).
  • Unit tests have been added for the changes (if applicable).
  • Breaking changes have been documented (or no breaking changes introduced in this PR).

@univer-bot univer-bot bot added the qa:untested This PR is ready to be tested label Jun 12, 2024
@jikkai jikkai requested a review from yuhongz June 13, 2024 02:48
Copy link

github-actions bot commented Jun 13, 2024

View Deployment

📑 Examples 📚 Storybook
🔗 Preview link 🔗 Preview link

@yuhongz
Copy link
Contributor

yuhongz commented Jun 13, 2024

感谢贡献代码!
代码还需要完善,此处如果去掉等号,冻结的上方的行往下插入一行时同样会导致不符合预期的表现。因此,如果要精细化处理此场景,应该基于锚点的位置,分别处理InsertRowAfterCommand 和 InsertRowBeforeCommand的两个场景,从而去决定是否要update。列也同理。 @semmywong

@univer-bot
Copy link

univer-bot bot commented Jun 13, 2024

Bot detected the issue body's language is not English, translate it automatically. 👯👭🏻🧑‍🤝‍🧑👫🧑🏿‍🤝‍🧑🏻👩🏾‍🤝‍👨🏿👬🏿

Thanks for contributing the code!
The code still needs to be improved. If the equal sign is removed here, inserting a row below the frozen row will also lead to unexpected performance. Therefore, if you want to handle this scene in detail, you should handle the two scenes of InsertRowAfterCommand and InsertRowBeforeCommand based on the position of the anchor point to decide whether to update. The same goes for columns. @semmywong

@zhaolixin7 zhaolixin7 removed the qa:untested This PR is ready to be tested label Jun 14, 2024
@semmywong
Copy link
Contributor Author

semmywong commented Jun 14, 2024

感谢贡献代码! 代码还需要完善,此处如果去掉等号,冻结的上方的行往下插入一行时同样会导致不符合预期的表现。因此,如果要精细化处理此场景,应该基于锚点的位置,分别处理InsertRowAfterCommand 和 InsertRowBeforeCommand的两个场景,从而去决定是否要update。列也同理。 @semmywong

@yuhongz 你好,我的理解,如果锚点正好在冻结的最后一行(也就是freeze.startRow - 1),进行InsertRowAfterCommand 应该是和冻结的最后一行的下一行(也就是freeze.startRow)进行InsertRowBeforeCommand一样的效果,也就是新增的行不应该被冻结的。
以下是我在腾讯文档上面的测试结果gif图(wps上面测试了也是同样效果):
240614 154229

@univer-bot
Copy link

univer-bot bot commented Jun 14, 2024

Bot detected the issue body's language is not English, translate it automatically. 👯👭🏻🧑‍🤝‍🧑👫🧑🏿‍🤝‍🧑🏻👩🏾‍🤝‍👨🏿👬🏿

Thanks for contributing the code! The code still needs to be improved. If the equal sign is removed here, inserting a row below the frozen row will also lead to unexpected performance. Therefore, if you want to handle this scene in detail, you should handle the two scenes of InsertRowAfterCommand and InsertRowBeforeCommand based on the position of the anchor point to decide whether to update. The same goes for columns. @semmywong

@yuhongz Hello, my understanding is that if the anchor point is exactly on the last frozen row (that is, freeze.startRow - 1), performing InsertRowAfterCommand should be the same as performing InsertRowBeforeCommand on the next row of the frozen last row (that is, freeze.startRow) The effect is that the newly added rows should not be frozen.
The following is the gif of my test results on Tencent documents (the same effect was tested on wps):
240614 154229

@semmywong
Copy link
Contributor Author

@yuhongz 哦,有一种情况,如果freeze.startRow === range.startRow + 1时,且进行InsertRowBeforeCommand要更新冻结数据

@univer-bot
Copy link

univer-bot bot commented Jun 14, 2024

Bot detected the issue body's language is not English, translate it automatically. 👯👭🏻🧑‍🤝‍🧑👫🧑🏿‍🤝‍🧑🏻👩🏾‍🤝‍👨🏿👬🏿

@yuhongz Oh, there is a situation, if freeze.startRow === range.startRow + 1, and InsertRowBeforeCommand is performed to update the frozen data

Copy link
Contributor

@yuhongz yuhongz left a comment

Choose a reason for hiding this comment

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

LGTM.

@yuhongz yuhongz merged commit 7c9d69a into dream-num:dev Jun 19, 2024
9 checks passed
Jocs added a commit that referenced this pull request Sep 10, 2024
Jocs added a commit that referenced this pull request Sep 10, 2024
Jocs added a commit that referenced this pull request Sep 10, 2024
Jocs added a commit that referenced this pull request Sep 11, 2024
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.

[Bug] update freeze incorrect when insert row
3 participants