-
Notifications
You must be signed in to change notification settings - Fork 7.1k
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
chore: remove useless fixedHeader
prop for Page
#5069
Conversation
|
Warning There were issues while running some tools. Please review the errors and either fix the tool’s configuration or disable the tool if it’s a critical failure. 🔧 eslint
packages/effects/common-ui/src/components/page/page.vueOops! Something went wrong! :( ESLint: 9.16.0 Error [ERR_MODULE_NOT_FOUND]: Cannot find module '/node_modules/@vben/eslint-config/dist/index.mjs' imported from /eslint.config.mjs WalkthroughThe pull request involves the removal of the Changes
Possibly related PRs
Suggested reviewers
Poem
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (4)
💤 Files with no reviewable changes (3)
🔇 Additional comments (4)packages/effects/common-ui/src/components/page/page.vue (4)
The constant Line range hint The removal of the
The changes enhance the layout by:
Line range hint The height calculation has been simplified to use ✅ Verification successfulHeight calculation is correctly implemented and consistent with the layout system The verification shows that:
🏁 Scripts executedThe following scripts were executed for the analysis: Script: #!/bin/bash
# Search for other usages of CSS_VARIABLE_LAYOUT_CONTENT_HEIGHT to verify consistency
rg "CSS_VARIABLE_LAYOUT_CONTENT_HEIGHT" --type vue --type ts -A 2
# Search for any height-related calculations that might be affected
rg "calc\(.*height" --type vue -A 2
Length of output: 159 Script: #!/bin/bash
# Search for other usages of CSS_VARIABLE_LAYOUT_CONTENT_HEIGHT without file type restrictions
rg "CSS_VARIABLE_LAYOUT_CONTENT_HEIGHT" -A 2
# Search for any height-related calculations in vue files (using file extension)
rg "calc\(.*height" "*.vue" -A 2
# Find all vue files that might contain height calculations
fd ".vue$" --exec grep -l "height"
# Look for layout-related constants or variables
rg "CSS_VARIABLE_LAYOUT" -A 2
Length of output: 7629 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: 1
🧹 Outside diff range and nitpick comments (3)
packages/effects/common-ui/src/components/page/page.vue (3)
74-76
: Consider improving template indentation.The class concatenation could be formatted more clearly:
- 'bg-card border-border relative flex items-end border-b px-6 py-4', - headerClass, + 'bg-card border-border relative flex items-end border-b px-6 py-4', + headerClass,
36-39
: Remove commented code.The commented overflow-y style should be removed if it's no longer needed.
- // 'overflow-y': shouldAutoHeight.value?'auto':'unset',
Line range hint
51-54
: Consider documenting the setTimeout usage.The 30ms delay in shouldAutoHeight assignment might need explanation for future maintainers.
+ // Allow time for the DOM to stabilize before enabling auto-height setTimeout(() => { shouldAutoHeight.value = true; }, 30);
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (4)
docs/src/components/layout-ui/page.md
(0 hunks)packages/effects/common-ui/src/components/page/page.vue
(5 hunks)playground/src/views/examples/form/basic.vue
(0 hunks)playground/src/views/examples/modal/index.vue
(0 hunks)
💤 Files with no reviewable changes (3)
- playground/src/views/examples/modal/index.vue
- docs/src/components/layout-ui/page.md
- playground/src/views/examples/form/basic.vue
🔇 Additional comments (2)
packages/effects/common-ui/src/components/page/page.vue (2)
79-91
: LGTM! Improved layout structure with flex container.
The addition of flex-auto wrapper improves content distribution and maintains proper layout after removing fixedHeader.
Line range hint 7-17
: Verify removal of fixedHeader references.
The Props interface changes look good. Let's verify no other references remain:
✅ Verification successful
No remaining references to fixedHeader
found in the codebase
The search across all files, specifically Vue and TypeScript files, returned no results, indicating that all references to fixedHeader
have been successfully removed from the codebase. This confirms that the Props interface changes are complete and consistent with the rest of the codebase.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check for any remaining fixedHeader references in the codebase
rg -i "fixedHeader" --type vue --type ts
Length of output: 68
Script:
#!/bin/bash
# Search for fixedHeader references in all files
rg -i "fixedHeader"
Length of output: 19
Script:
#!/bin/bash
# Find all Vue files and search for fixedHeader
fd -e vue -x rg -i "fixedHeader" {}
Length of output: 37
Script:
#!/bin/bash
# Find all TypeScript files and search for fixedHeader
fd -e ts -x rg -i "fixedHeader" {}
Length of output: 36
daf83e3
to
6536921
Compare
Description
移除
Page
组件中已经无用的fixedHeader
属性Type of change
Please delete options that are not relevant.
pnpm-lock.yaml
unless you introduce a new test example.Checklist
pnpm run docs:dev
command.pnpm test
.feat:
,fix:
,perf:
,docs:
, orchore:
.Summary by CodeRabbit
Release Notes
Documentation
Page
component documentation to reflect the removal of thefixedHeader
property.New Features
Page
component by removing thefixedHeader
property, streamlining the component's props and style calculations.Improvements
Page
component for better layout management.Bug Fixes
fixed-header
attribute from the modal examples to ensure consistent layout behavior.