feat(dialog-select): [dialog-select] add attribute lock-scroll #3489
Conversation
WalkthroughA new Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant DialogSelect
participant DialogBox
User->>DialogSelect: Open dialog (with lockScroll prop)
DialogSelect->>DialogBox: Render with lock-scroll = lockScroll
alt lockScroll = true
DialogBox->>Document: Add scroll lock class to body
else lockScroll = false
DialogBox->>Document: Do not add scroll lock class
end
Poem
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
examples/sites/demos/apis/dialog-select.jsOops! Something went wrong! :( ESLint: 8.57.1 ESLint couldn't find the plugin "eslint-plugin-vue". (The package "eslint-plugin-vue" was not found when loaded as a Node module from the directory "".) It's likely that the plugin isn't installed correctly. Try reinstalling by running the following: The plugin "eslint-plugin-vue" was referenced from the config file in ".eslintrc.js » @antfu/eslint-config » @antfu/eslint-config-vue". If you still can't figure out the problem, please stop by https://eslint.org/chat/help to chat with the team. examples/sites/demos/pc/app/dialog-select/nest-grid-multi-composition-api.vueOops! Something went wrong! :( ESLint: 8.57.1 ESLint couldn't find the plugin "eslint-plugin-vue". (The package "eslint-plugin-vue" was not found when loaded as a Node module from the directory "".) It's likely that the plugin isn't installed correctly. Try reinstalling by running the following: The plugin "eslint-plugin-vue" was referenced from the config file in ".eslintrc.js » @antfu/eslint-config » @antfu/eslint-config-vue". If you still can't figure out the problem, please stop by https://eslint.org/chat/help to chat with the team. examples/sites/demos/pc/app/dialog-select/nest-grid-multi.vueOops! Something went wrong! :( ESLint: 8.57.1 ESLint couldn't find the plugin "eslint-plugin-vue". (The package "eslint-plugin-vue" was not found when loaded as a Node module from the directory "".) It's likely that the plugin isn't installed correctly. Try reinstalling by running the following: The plugin "eslint-plugin-vue" was referenced from the config file in ".eslintrc.js » @antfu/eslint-config » @antfu/eslint-config-vue". If you still can't figure out the problem, please stop by https://eslint.org/chat/help to chat with the team.
✨ Finishing Touches
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. 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 (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
examples/sites/demos/pc/app/dialog-select/nest-grid-multi.spec.ts (1)
9-9: Consider adding a wait condition for dialog visibility.The assertion checks for the absence of the scroll-lock class immediately after clicking the button. While this appears correct for the
lock-scroll="false"case, consider adding a wait condition to ensure the dialog has fully opened before making the assertion, as dialog opening might involve asynchronous operations.await demo.getByRole('button', { name: '打开窗口' }).click() + await expect(page.locator('.tiny-dialog-box')).toBeVisible() await expect(page.locator('body')).not.toHaveClass(/dialog-box__scroll-lock/)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
examples/sites/demos/apis/dialog-select.js(1 hunks)examples/sites/demos/pc/app/dialog-select/nest-grid-multi-composition-api.vue(1 hunks)examples/sites/demos/pc/app/dialog-select/nest-grid-multi.spec.ts(1 hunks)examples/sites/demos/pc/app/dialog-select/nest-grid-multi.vue(1 hunks)examples/sites/demos/pc/app/dialog-select/webdoc/dialog-select.js(1 hunks)packages/vue/src/dialog-select/src/index.ts(1 hunks)packages/vue/src/dialog-select/src/pc.vue(2 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: PR E2E Test (pnpm test:e2e3)
🔇 Additional comments (6)
examples/sites/demos/pc/app/dialog-select/nest-grid-multi.vue (1)
27-27: LGTM! Clean implementation of the lock-scroll feature.The property binding correctly demonstrates the new
lock-scrollfeature by explicitly setting it tofalse. This follows Vue.js conventions for prop naming (kebab-case in templates, camelCase in JavaScript).examples/sites/demos/pc/app/dialog-select/nest-grid-multi-composition-api.vue (1)
27-27: LGTM! Consistent implementation across API styles.The
lock-scrollproperty binding is correctly implemented and maintains consistency with the options API demo version. This ensures both Vue.js API styles demonstrate the feature properly.packages/vue/src/dialog-select/src/index.ts (1)
86-88:✅ Verification successful
LGTM! Proper prop implementation with backward compatibility.
The
lockScrollprop is correctly implemented with:
- Proper TypeScript typing (Boolean)
- Sensible default value (
true) that maintains backward compatibility- Standard camelCase naming convention
Let me verify that this prop is properly wired through to the template:
🏁 Script executed:
#!/bin/bash # Description: Verify that the lockScroll prop is used in the PC template # Expected: The prop should be passed to the dialog-box component # Search for lockScroll usage in the pc.vue template ast-grep --pattern 'lock-scroll' # Also check for any camelCase usage ast-grep --pattern 'lockScroll'Length of output: 64
I’m not seeing any occurrences in the initial AST search. Let’s try a broader grep across all Vue templates:
🏁 Script executed:
#!/bin/bash # Search for lockScroll usage in any .vue file rg -n "lockScroll" --glob "*.vue" rg -n "lock-scroll" --glob "*.vue"Length of output: 6116
Verified
lockScrollprop wiringThe
lockScrollprop is correctly declared and passed through in the PC template:
- In packages/vue/src/dialog-select/src/pc.vue (line 17):
:lock-scroll="lockScroll"No further changes needed.
packages/vue/src/dialog-select/src/pc.vue (1)
17-17: LGTM! Correct template binding.The kebab-case binding
:lock-scroll="lockScroll"follows Vue.js conventions and correctly passes the prop to the underlying dialog-box component.examples/sites/demos/apis/dialog-select.js (1)
34-47: LGTM! Well-documented API addition.The
lock-scrollprop documentation is comprehensive with:
- Correct type definition (boolean)
- Appropriate default value (true)
- Clear descriptions in both languages
- Proper meta information indicating stability since 3.24.0
- Consistent demo reference
examples/sites/demos/pc/app/dialog-select/webdoc/dialog-select.js (1)
15-17: LGTM! Clear documentation enhancement.The updated documentation clearly explains the
lock-scrollattribute functionality in both languages, helping users understand when and how to use this feature to control scrollbar behavior in popup dialogs.
| 'mainHeight', | ||
| 'lockScroll' |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add prop definition with type and default value.
The prop should be defined with proper type and default value to match the API documentation which specifies boolean type with default true.
Replace the string-based prop definition with an object-based definition:
- 'mainHeight',
- 'lockScroll'
+ 'mainHeight',
+ {
+ name: 'lockScroll',
+ type: Boolean,
+ default: true
+ }Or alternatively, define all props using the object syntax for better type safety and consistency.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| 'mainHeight', | |
| 'lockScroll' | |
| 'mainHeight', | |
| { | |
| name: 'lockScroll', | |
| type: Boolean, | |
| default: true | |
| } |
🤖 Prompt for AI Agents
In packages/vue/src/dialog-select/src/pc.vue at lines 220 to 221, the props
'mainHeight' and 'lockScroll' are currently defined as strings without type or
default values. Update the prop definitions to use the object syntax, specifying
the type as Boolean and setting the default value to true, to align with the API
documentation and improve type safety and consistency.
PR
PR Checklist
Please check if your PR fulfills the following requirements:
PR Type
What kind of change does this PR introduce?
What is the current behavior?
Issue Number: N/A
What is the new behavior?
Does this PR introduce a breaking change?
Other information
Summary by CodeRabbit