-
Notifications
You must be signed in to change notification settings - Fork 308
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(Table): missing scroll after toggle CardView mode #4600
Conversation
Reviewer's Guide by SourceryThis PR fixes a scrolling issue when toggling between CardView and TableView modes in the Table component. The implementation includes proper cleanup of table resources during view changes and improvements to null handling using nullish coalescing operators. The changes ensure scroll functionality is maintained when switching view modes and table resources are properly managed. Sequence diagram for toggling between CardView and TableViewsequenceDiagram
actor User
participant TableComponent
participant JavaScript
User ->> TableComponent: Click CardView button
TableComponent ->> JavaScript: toggleView(id)
JavaScript ->> JavaScript: destroyTable(table)
JavaScript ->> JavaScript: reset(id)
JavaScript ->> TableComponent: Update view
TableComponent ->> User: Display updated view with scroll
Updated class diagram for Table componentclassDiagram
class Table {
- bool _viewChanged
+ void OnClickCardView()
+ Task OnAfterRenderAsync(bool firstRender)
}
note for Table "Added _viewChanged to track view changes"
note for Table "OnClickCardView now sets _viewChanged to true"
note for Table "OnAfterRenderAsync checks _viewChanged and calls toggleView if true"
File-Level Changes
Assessment against linked issues
Possibly linked issues
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hey @ArgoZhang - I've reviewed your changes - here's some feedback:
Overall Comments:
- Consider fixing the spelling of 'destoryTable' to 'destroyTable' for consistency
Here's what I looked at during the review
- 🟡 General issues: 1 issue found
- 🟢 Security: all looks good
- 🟢 Testing: all looks good
- 🟢 Complexity: all looks good
- 🟢 Documentation: all looks good
Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #4600 +/- ##
=========================================
Coverage 100.00% 100.00%
=========================================
Files 620 620
Lines 27390 27396 +6
Branches 3920 3921 +1
=========================================
+ Hits 27390 27396 +6 ☔ View full report in Codecov by Sentry. |
@sourcery-ai review |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hey @ArgoZhang - I've reviewed your changes and they look great!
Here's what I looked at during the review
- 🟡 General issues: 1 issue found
- 🟢 Security: all looks good
- 🟢 Testing: all looks good
- 🟢 Complexity: all looks good
- 🟢 Documentation: all looks good
Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
missing scroll after toggle CardView mode
Summary of the changes (Less than 80 chars)
简单描述你更改了什么, 不超过80个字符;如果有关联 Issue 请在下方填写相关编号
Description
fixes #4574
Regression?
[If yes, specify the version the behavior has regressed from]
[是否影响老版本]
Risk
[Justify the selection above]
Verification
Packaging changes reviewed?
☑️ Self Check before Merge
Summary by Sourcery
Fix the missing scroll issue when toggling CardView mode in the Table component and enhance the component by adding a toggleView function and using nullish coalescing for better code reliability.
Bug Fixes:
Enhancements: