docs: fix demo anchor position not correct #2391
Conversation
WalkthroughThe changes involve updates to documentation and code related to component imports in a Vue.js application. Key modifications include the introduction of a "Tiny" prefix for component names, adjustments to import statements, and enhancements to menu generation and demo component functionality. New methods were added to improve the synchronization of demo components, and a new route for documentation was established in the Vue Router setup. Changes
Possibly related PRs
Suggested labels
Suggested reviewers
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 (
|
|
[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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Outside diff range and nitpick comments (5)
examples/sites/src/router.js (1)
20-26: LGTM with suggestions for improvement.The new documentation route follows the established pattern and integrates well with the existing router configuration.
Consider these improvements:
- Add meta title for consistency with other routes:
{ path: `${context}:all?/:lang/:theme/docs/:docId`, component: Layout, name: 'docs', - children: [{ name: 'Docs', path: '', component: Docs }] + children: [{ + name: 'Docs', + path: '', + component: Docs, + meta: { title: '文档 | TinyVue' } + }] }
- Add route guards to validate language and theme parameters:
beforeEnter: (to, from) => { const { lang, theme } = to.params if (!LANG_PATH_MAP[lang]) { return { path: to.path.replace(lang, ZH_CN_LANG) } } if (!THEME_ROUTE_MAP[theme]) { return { path: to.path.replace(theme, SMB_THEME) } } return true }examples/sites/demos/pc/webdoc/import-components.md (1)
87-92: Consider enhancing the tip block's accessibility.The content and placement of the version compatibility notice is excellent. However, consider improving the HTML structure for better accessibility.
<div class="tip custom-block"> -<br /> -温馨提示：带有Tiny前缀的组件导出自3.17.0开始支持。若使用之前版本，需使用别名。 -<br /> -<p>例如：<code>import { Button as TinyButton } from '@opentiny/vue'</code></p> +<p>温馨提示：带有Tiny前缀的组件导出自3.17.0开始支持。若使用之前版本，需使用别名。</p> +<p>例如：<code>import { Button as TinyButton } from '@opentiny/vue'</code></p> </div>examples/sites/src/views/components/demo.vue (1)
217-219: LGTM: Proper event emission timing.The mounted event is correctly emitted after nextTick, ensuring the DOM is fully updated. This is crucial for accurate anchor positioning in the parent component.
This implementation ensures proper synchronization between demo component mounting and anchor positioning by:
- Waiting for the component to mount
- Ensuring DOM updates are complete via nextTick
- Notifying the parent component at the right moment
examples/sites/src/views/components/components.vue (2)
228-228: Consider makingoffset-topdynamic instead of hardcodedThe hardcoded value
:offset-top="156"may lead to maintenance issues if the layout changes. Consider computing the offset dynamically based on the header's height to make the component more adaptable.You could calculate
offsetTopin the script section and use it in the template:const offsetTop = ref(156) // Replace 156 with a computed value if possibleAnd in the template:
<tiny-anchor id="anchor" :offset-top="offsetTop" :links="currAnchorLinks" :key="anchorRefreshKey" :is-affix="anchorAffix" type="dot" mask-class="custom-active-anchor" container-id="#doc-layout" @link-click="handleAnchorClick" > </tiny-anchor>
594-596: Add error handling for the Promise returned byallDemoMountedCurrently, any errors occurring in
allDemoMounted()may go unhandled, potentially causing uncaught exceptions. Consider adding error handling to manage potential errors gracefully.Modify the Promise chain to include a catch block:
return allDemoMounted().then(() => { scrollByHash(hash) + }).catch((error) => { + console.error('Error while waiting for all demos to mount:', error) + })
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (6)
- examples/sites/demos/pc/webdoc/import-components-en.md (2 hunks)
- examples/sites/demos/pc/webdoc/import-components.md (2 hunks)
- examples/sites/src/menus.jsx (3 hunks)
- examples/sites/src/router.js (1 hunks)
- examples/sites/src/views/components/components.vue (5 hunks)
- examples/sites/src/views/components/demo.vue (2 hunks)
🔇 Additional comments (12)
examples/sites/src/router.js (1)
22-22: Verify the relationship between route changes and anchor position fix.The PR objective mentions fixing demo anchor positions, but it's not immediately clear how this route change contributes to that fix. Could you please clarify if this is part of a larger solution?
Let's check for related changes:
examples/sites/src/menus.jsx (3)
1-1: LGTM: Import statement is correctly updatedThe addition of
docMenusimport is consistent with the new functionality and follows the existing code style.
49-49: LGTM: Menu concatenation order is logicalThe return statement correctly combines all menu types in a sensible order: overview, documentation, and components.
Line range hint
1-49: Consider implementing scroll position managementWhile the menu structure improvements are good, fixing demo anchor positions might also require:
- Implementation of scroll position management in the router
- Proper handling of anchor links in the documentation views
- Coordination with the router's scrollBehavior option
Let's check if scroll behavior is properly configured:
examples/sites/demos/pc/webdoc/import-components-en.md (2)
47-52: LGTM! Clear and helpful version compatibility notice.The tip block effectively communicates the version requirement for Tiny prefix support and provides a practical example for users of earlier versions.
65-65: LGTM! Consistent implementation of the new naming convention.The import statement and component registration correctly demonstrate the new Tiny prefix approach, maintaining consistency with both the template usage and the version guidance.
Also applies to: 69-70
examples/sites/demos/pc/webdoc/import-components.md (2)
94-94: LGTM! The import example correctly demonstrates the new naming convention.The updated example properly shows the direct use of Tiny-prefixed components, which aligns with the version 3.17.0+ requirements.
Also applies to: 105-105, 109-110
Line range hint
1-1: Please verify the anchor position fix.The
<!--anchor:on-->comment appears to be related to the PR's objective of fixing demo anchor positions. Could you verify:
- How this comment affects the anchor positioning?
- Have you tested that this fix resolves the original issue?
✅ Verification successful
Based on the search results, I can see that the
<!--anchor:on-->comment appears to be a configuration for markdown-it-anchor plugin used in the documentation site. The plugin is used to generate anchor links for headings in markdown files. The search results also show that this configuration is consistently used across multiple documentation files.The anchor position fix has been implemented correctly.
The
<!--anchor:on-->comment enables the markdown-it-anchor plugin functionality for that section of the documentation, which allows proper anchor link generation and navigation. This matches the PR's objective of fixing demo anchor positions.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for other anchor-related configurations or similar patterns rg -g '!*.{log,lock}' -i 'anchor:(on|off)' # Look for any documentation about anchor configuration fd -g 'README*' -X grep -i 'anchor'Length of output: 545
Script:
#!/bin/bash # Search for any JavaScript/TypeScript files that might handle these anchor comments rg -g '*.{js,ts,vue}' -B 2 -A 2 'anchor' # Look for any documentation files that might explain the anchor functionality fd -e md -X grep -l -i 'anchor' | xargs cat # Search for any configuration files that might handle anchors fd -e json -e yaml -e yml -X grep -i 'anchor'Length of output: 235002
examples/sites/src/views/components/demo.vue (2)
95-95: LGTM: Proper emit declaration.The 'mounted' event is correctly declared in the component's emits option, following Vue 3 best practices.
103-103: LGTM: Proper setup function signature.The emit parameter is correctly destructured from the setup context, following Vue 3 Composition API patterns.
examples/sites/src/views/components/components.vue (2)
639-639: LGTMAdding
demoMountedto thefnobject makes the method accessible within the template. This change is appropriate and aligns with Vue's composition API practices.
110-110: Verify that thedemocomponent emits themountedeventThe addition of
@mounted="demoMounted"expects thedemocomponent to emit amountedevent. Please ensure that thedemocomponent emits this event so that thedemoMountedmethod functions correctly.Run the following script to confirm that the
democomponent emits themountedevent:✅ Verification successful
The
mountedevent is properly implemented in the demo componentThe verification confirms that the
democomponent correctly implements and emits themountedevent:
- The component explicitly declares
mountedin itsemitsoption:emits: ['mounted']- The event is emitted after component mounting using
emit('mounted')within anextTickcallback- This matches the parent's usage of
@mounted="demoMounted"🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify that the `demo.vue` component emits the `mounted` event. # Expectation: The `demo.vue` file should contain a line emitting the 'mounted' event. rg 'this\.\$emit\(\s*[\'"]mounted[\'"]\s*\)' $(fd demo.vue)Length of output: 93
Script:
#!/bin/bash # Let's first find the demo component file fd -t f demo.vue # Then let's examine its content for any mounted-related code rg -A 5 -B 5 'mounted|emit' $(fd demo.vue) # Also check for lifecycle hooks pattern ast-grep --pattern 'mounted() { $$$ }'Length of output: 2661
| const docOptions = docMenus.map((menu) => ({ | ||
| ...menu, | ||
| label: `${appData.lang === 'zhCN' ? menu.label : menu.labelEn}${getChildrenStr(menu)}`, | ||
| children: menu.children.map((page) => ({ | ||
| ...page, | ||
| id: page.key, | ||
| label: appData.lang === 'zhCN' ? page.title : page.titleEn, | ||
| type: 'docs' | ||
| })) | ||
| })) |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add safety checks and consider type constants
While the logic is sound, consider these improvements for robustness:
- Add null checks for menu.children
- Consider extracting 'docs' type to a constant
Here's a suggested improvement:
+const MENU_TYPES = {
+ DOCS: 'docs',
+ COMPONENTS: 'components',
+ OVERVIEW: 'overview'
+}
const docOptions = docMenus.map((menu) => ({
...menu,
label: `${appData.lang === 'zhCN' ? menu.label : menu.labelEn}${getChildrenStr(menu)}`,
- children: menu.children.map((page) => ({
+ children: (menu.children || []).map((page) => ({
...page,
id: page.key,
label: appData.lang === 'zhCN' ? page.title : page.titleEn,
- type: 'docs'
+ type: MENU_TYPES.DOCS
}))
}))📝 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.
| const docOptions = docMenus.map((menu) => ({ | |
| ...menu, | |
| label: `${appData.lang === 'zhCN' ? menu.label : menu.labelEn}${getChildrenStr(menu)}`, | |
| children: menu.children.map((page) => ({ | |
| ...page, | |
| id: page.key, | |
| label: appData.lang === 'zhCN' ? page.title : page.titleEn, | |
| type: 'docs' | |
| })) | |
| })) | |
| const MENU_TYPES = { | |
| DOCS: 'docs', | |
| COMPONENTS: 'components', | |
| OVERVIEW: 'overview' | |
| } | |
| const docOptions = docMenus.map((menu) => ({ | |
| ...menu, | |
| label: `${appData.lang === 'zhCN' ? menu.label : menu.labelEn}${getChildrenStr(menu)}`, | |
| children: (menu.children || []).map((page) => ({ | |
| ...page, | |
| id: page.key, | |
| label: appData.lang === 'zhCN' ? page.title : page.titleEn, | |
| type: MENU_TYPES.DOCS | |
| })) | |
| })) |
| let finishNum = 0 | ||
| let isAllMounted = false | ||
| let demoMountedResolve | ||
| const demoMounted = () => { | ||
| finishNum++ | ||
| if (finishNum === state.currJson.demos.length) { | ||
| isAllMounted = true | ||
| demoMountedResolve(true) | ||
| } | ||
| } | ||
|
|
||
| const allDemoMounted = async () => { | ||
| if (isAllMounted) { | ||
| return isAllMounted | ||
| } | ||
| return new Promise((resolve) => { | ||
| demoMountedResolve = resolve | ||
| }) | ||
| } |
There was a problem hiding this comment.
Ensure mounting state variables are reset appropriately
Variables finishNum, isAllMounted, and demoMountedResolve may retain their values across multiple invocations of loadPage, potentially causing incorrect behavior when navigating between components or reloading. Reset these variables when loadPage is called to maintain correct state.
Apply this diff to reset the variables at the beginning of the loadPage function:
const loadPage = debounce(templateModeState.isSaas ? 100 : 0, false, () => {
+ finishNum = 0
+ isAllMounted = false
+ demoMountedResolve = null
const lang = getWord('cn', 'en')
state.cmpId = router.currentRoute.value.params.cmpIdCommittable suggestion was skipped due to low confidence.
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
Release Notes
New Features
Bug Fixes
Documentation