-
Notifications
You must be signed in to change notification settings - Fork 77
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: UI backups list hides the "next 10" buttons #858
Conversation
Signed-off-by: Yi-Ya Chen <yiya.chen@suse.com>
Signed-off-by: Yi-Ya Chen <yiya.chen@suse.com>
WalkthroughThe pull request introduces styling modifications across multiple backup-related React components and their associated Less stylesheet. The changes primarily focus on adjusting element overflow, removing specific class names, and standardizing height and layout properties for table and container elements. These modifications aim to improve the visual consistency and layout behavior of backup-related views without altering the underlying component logic. Changes
Poem
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
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.
Actionable comments posted: 0
🧹 Nitpick comments (1)
src/routes/backup/backupList.less (1)
20-41
: Excellent solution for fixing pagination visibility!The implementation effectively solves the pagination visibility issue by:
- Ensuring proper height inheritance through the component hierarchy
- Explicitly reserving space for pagination (40px)
- Preventing unwanted scrolling behavior
This is a robust solution that maintains consistency across both backup list and detail views.
Consider documenting the 40px pagination height as a CSS variable for better maintainability:
.ant-table { - height: calc(100% - 40px); // pagination height + height: calc(100% - var(--pagination-height, 40px)); }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
src/routes/backup/BackupDetail.js
(1 hunks)src/routes/backup/BackupList.js
(1 hunks)src/routes/backup/BackupVolumeList.js
(1 hunks)src/routes/backup/backupList.less
(1 hunks)
✅ Files skipped from review due to trivial changes (3)
- src/routes/backup/BackupVolumeList.js
- src/routes/backup/BackupDetail.js
- src/routes/backup/BackupList.js
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: Summary
🔇 Additional comments (2)
src/routes/backup/backupList.less (2)
1-18
: LGTM! Properly formatted container styles.The workloadContainer class maintains proper styling for container and pod elements with appropriate positioning and text overflow handling.
20-41
: Verify the changes in development environment.Please confirm that these style changes resolve the pagination visibility issue by testing with:
- Tables with varying amounts of data
- Different screen sizes
- Both backup list and detail views
Use the provided command to test:
✅ Verification successful
Style changes are properly implemented and consistent with codebase patterns
The table height calculation reserving 40px for pagination follows the established patterns in the codebase, and the implementation correctly handles:
- Table container height inheritance
- Pagination visibility
- Scroll behavior
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
LONGHORN_MANAGER_IP=http://134.209.101.112:30001/ npm run devLength of output: 204
Script:
#!/bin/bash # Find files containing backTable and backDetailTable echo "=== Files containing table IDs ===" rg -l "backTable|backDetailTable" --type-add 'style:*.{less,css}' --type-add 'template:*.{js,jsx,ts,tsx}' echo -e "\n=== Other table height definitions ===" rg "height: calc\(.*px\)" --type=style echo -e "\n=== Pagination related styles ===" rg "pagination" --type=style -C 2Length of output: 601
Script:
#!/bin/bash # Check height calculations and pagination styles without file type restriction echo "=== Height calculations in styles ===" rg "height:\s*calc\(" --type=less echo -e "\n=== Pagination related styles ===" rg "pagination" --type=less -C 2 echo -e "\n=== Table component implementations ===" cat src/routes/backup/BackupVolumeList.js src/routes/backup/BackupList.jsLength of output: 24443
@mergify backport v1.8.x |
✅ Backports have been created
|
What this PR does / why we need it
The user cannot navigate to the next page because the pagination is not visible.
This PR adjusts the styles to ensure the pagination is displayed properly.
Issue
[BUG] UI Backups list hides the "next 10" buttons #10211
Test Result
Volume backup page
Volume backup detail page
Additional documentation or context
Test with
LONGHORN_MANAGER_IP=http://134.209.101.112:30001/ npm run dev
Summary by CodeRabbit
Style Updates
common-table-class
from multiple backup-related componentsRefactor