-
Notifications
You must be signed in to change notification settings - Fork 273
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(tabs): modify the issue of the mobile demo not displaying and reporting errors #2710
Conversation
WalkthroughThe pull request focuses on refactoring the tabs component's renderless implementation across two files: Changes
Possibly related PRs
Suggested labels
Suggested reviewers
Poem
Tip CodeRabbit's docstrings feature is now available as part of our Early Access Program! Simply use the command 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 (
|
[e2e-test-warn] The title of the Pull request should look like "fix(vue-renderless): [action-menu, alert] fix xxx bug". Please make sure you've read our contributing guide |
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)
packages/mobile/components/tabs/src/renderless/index.ts (1)
71-71
: Check for reactivity and layout reflows.
Usingvm.$el
is appropriate here. Make sure that any changes to the DOM structure or dynamic rerendering do not adversely impact performance in high-update scenarios.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
packages/mobile/components/tabs/src/renderless/index.ts
(4 hunks)packages/mobile/components/tabs/src/renderless/vue.ts
(2 hunks)
🔇 Additional comments (11)
packages/mobile/components/tabs/src/renderless/vue.ts (4)
105-105
: Use vm
for context consistency.
Replacing parent
with vm
in the parameters clarifies that the Vue instance itself is being referenced rather than its parent, which aligns with best practices for renderless components.
116-117
: Ensure references to vm
are valid in child components.
Functions like calcMorePanes
and calcExpandPanes
now rely on vm
for slots and element references. Make sure any child references (vm.$slots.default
, vm.$el
) remain valid, especially if migrated from SFC setups or composition APIs.
124-124
: Refactor maintains logical consistency.
Using vm
for the created
lifecycle hook ensures the component references remain consistent with the rest of the code. No breaking changes detected here.
129-129
: Confirm that provide('rootTabs', vm)
meets usage expectations.
Switching from parent
to vm
in the provider object is valid. However, confirm that consumers of rootTabs
are prepared to receive the Vue instance instead of the parent instance.
packages/mobile/components/tabs/src/renderless/index.ts (7)
17-20
: Parameter signature change to vm
.
Replacing parent
with vm
in calcPaneInstances
ensures that $slots.default
references the current component’s scope. This is logically consistent with the rest of the refactor.
22-22
: Confirm $slots.default
usage.
Ensure that this refactor accurately reflects where the pane data are coming from. If the conversation was previously referencing the parent’s slots, verify that pulling from vm.$slots.default
doesn’t break existing usage.
65-65
: Maintain uniform usage of vm
.
Your changes within calcMorePanes
ensure that trackable elements reference the correct instance. No issues spotted.
96-96
: Stay consistent with earlier refactor.
Using vm
in calcExpandPanes
remains in line with the updated approach.
101-101
: Watch out for potential timing issues.
Accessing vm.$el
in this part of the lifecycle might introduce timing pitfalls if elements are not yet rendered. Ensure that onMounted or onUpdated handles any updates that rely on layout calculations.
188-188
: Intentional refactor from parent
to vm
.
The updated signature in created
consistently references the root instance instead of the parent, aligning well with the rest of the changes.
192-192
: Event binding now references the local component.
Substituting parent.$on
for vm.$on
ensures event listening is scoped to this instance. If the intent was to capture events from a parent or an ancestor, confirm that this is still the desired behavior.
…orting errors
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
Bug Fixes
Documentation