fix(drawer): [drawer] modify the problem that the before-close method is triggered upon page loading is solved #2364
Conversation
WalkthroughThe changes in this pull request primarily involve modifications to the Changes
Possibly related PRs
Suggested reviewers
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 (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (3)
packages/renderless/src/drawer/index.ts (3)
36-38: Add test coverage for the modified close behavior.The change in default behavior needs test coverage to verify:
- The fix for unwanted
beforeClosetriggers during page load- Existing close functionality still works when force is explicitly set to false
- The interaction with
maskClosableandbeforeClosepropsWould you like me to help create test cases for these scenarios?
36-38: Document the behavior change in component documentation.This change modifies the default behavior of the drawer's close method. Please update the component's documentation to reflect that:
beforeCloseis now bypassed by default- To maintain the previous behavior, force parameter needs to be explicitly set to false
Would you like me to help draft the documentation updates?
Line range hint
52-67: Consider refactoring handleClose for better maintainability.The current implementation mixes multiple concerns:
- Force close handling
- Mask close handling
- beforeClose validation
- Event emission
Consider splitting these into separate methods for better maintainability and testing.
Here's a suggested refactor:
const shouldPreventClose = ({ type, force, maskClosable, beforeClose }) => { if (force) return false if (type === 'mask' && !maskClosable) return true return typeof beforeClose === 'function' && beforeClose(type) === false } const emitCloseEvents = ({ type, emit }) => { emit('update:visible', false) emit(['close', 'confirm'].includes(type) ? type : 'close') } export const handleClose = ({ emit, props, state }: Pick<IDrawerRenderlessParams, 'emit' | 'props' | 'state'>) => (type: string, force?: boolean) => { if (shouldPreventClose({ type, force, ...props })) return if (state.visible) { state.visible = false setTimeout(() => emitCloseEvents({ type, emit }), 200) } }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (1)
- packages/renderless/src/drawer/index.ts (1 hunks)
🧰 Additional context used
🔇 Additional comments (1)
packages/renderless/src/drawer/index.ts (1)
36-38: Verify the impact of changing the default force parameter.While this change fixes the unwanted trigger of
beforeCloseduring page load, it represents a significant behavior change that might affect existing implementations.Let's verify the usage patterns in the codebase:
✅ Verification successful
Based on the search results, I can make the following observations:
Direct calls to
close()method:
- Most calls are either with no arguments or with explicit boolean values
- In drawer-specific examples:
examples/sites/demos/pc/app/drawer/before-close.vueandbefore-close-composition-api.vueexplicitly passtrue- Other drawer usages don't pass any argument
Event handlers:
- Many components use
@closeevent handlers- No evidence of code relying on the default
falsebehaviorLet me generate a final response:
The change to default
force=trueappears safe to implementThe codebase analysis shows that:
- Explicit calls to drawer's close method either pass no arguments or explicitly pass
true- The examples demonstrating
beforeClosefunctionality explicitly pass the force parameter- No code patterns were found that rely on the default
falsebehaviorThis change aligns with the common usage patterns in the codebase and should successfully fix the unwanted trigger of
beforeCloseduring page load without breaking existing implementations.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for direct calls to drawer close method rg -A 2 "\.close\(" # Search for drawer close event handlers rg -A 2 "@close|v-on:close|:on-close"Length of output: 34382
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (1)
examples/sites/demos/pc/app/drawer/before-close.spec.ts (1)
Line range hint
1-33: Test coverage appears incomplete for the reported bug.While this test covers the drawer's close behavior, it doesn't specifically test the reported bug where the "before-close" method was being triggered during page load.
Consider adding a test case that specifically verifies the "before-close" method is not triggered during page load. I can help draft this test case. Here's a suggested implementation:
test('before-close should not trigger on page load', async ({ page }) => { let beforeCloseCalled = false await page.exposeFunction('logBeforeClose', () => { beforeCloseCalled = true }) // Navigate and wait for page load await page.goto('drawer#before-close') await page.waitForLoadState('networkidle') // Verify before-close wasn't called during load expect(beforeCloseCalled).toBe(false) })Would you like me to create a GitHub issue to track this test addition?
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (1)
- examples/sites/demos/pc/app/drawer/before-close.spec.ts (1 hunks)
🧰 Additional context used
🔇 Additional comments (2)
examples/sites/demos/pc/app/drawer/before-close.spec.ts (2)
15-15: Verify the simplified assertion maintains test robustness.The removal of
.nth(1)suggests the DOM structure was simplified. Let's ensure this doesn't reduce test coverage.#!/bin/bash # Description: Check for other instances of nth() usage with modal messages # Expected: Consistent handling of modal message assertions # Search for other tests using nth() with modal messages echo "Checking for other nth() usage with modal messages..." rg -A 3 -B 3 'message.*nth\(' 'examples/sites/demos/pc/'
8-8: Verify the reliability of the new selector.The change from
.tiny-modal.type__messageto.tiny-modal__textsuggests a DOM structure change. Let's verify this selector is consistently used across the codebase.✅ Verification successful
Let me generate new shell scripts to better verify the selector changes.
This should help us understand:
- The actual DOM structure in templates
- How modal selectors are used across test files
- Specific usage in the drawer component context
Based on the search results, I can now generate a final response regarding the selector change:
The selector change is part of a consistent modal implementation pattern
The search results show that
.tiny-modal__textis used specifically in the drawer component's context, while the general modal pattern across the codebase uses variations like:
.tiny-modal__boxfor general modal boxes.tiny-modal__bodyfor modal content.tiny-modal__textfor text contentThe change from
.tiny-modal.type__messageto.tiny-modal__textaligns with the component's BEM-style naming convention and is more specific to the text content being tested.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check for consistency in modal text selectors # Expected: All modal text selectors should use the same pattern # Search for both old and new selectors to ensure complete migration echo "Checking for old selector usage..." rg -l "tiny-modal.type__message" echo "Checking for new selector usage..." rg -l "tiny-modal__text" # Check for any other similar modal text selectors echo "Checking for other modal text selector patterns..." rg -l "tiny-modal.*text"Length of output: 1153
Script:
#!/bin/bash # Search for modal-related class names in template files echo "Checking modal-related classes in template files..." rg -A 3 -B 3 'class=.*tiny-modal' --type html # Search for modal-related selectors in test files echo "Checking modal-related selectors in test files..." rg -A 3 -B 3 'locator\(.*(tiny-modal)' --type ts # Search specifically in the drawer component directory echo "Checking drawer component files..." fd -e vue -e ts -e js drawer$ -x rg -l "tiny-modal"Length of output: 49348
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
New Features
closemethod is invoked, enhancing usability.Improvements
Tests