docs: add common footer and optimize demos scroll #2485
Conversation
WalkthroughThe changes in this pull request introduce a new footer design, including the addition of multiple CSS classes for styling and layout. A new HTML structure for the footer is established within the JavaScript component, replacing a placeholder method. Additionally, several Vue components are updated to improve their functionality and responsiveness, including modifications to scroll handling and state management. Overall, the updates enhance the user interface and improve the organization of footer content. 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: 4
🧹 Outside diff range and nitpick comments (8)
examples/sites/src/views/components/float-settings.vue (2)
289-292: Enhance visual feedback consistency.The box shadow styling could be improved for better maintainability and user experience.
Consider these improvements:
.settings-btn { /* ... existing styles ... */ - box-shadow: 0 2px 4px 0 rgba(0, 0, 0, 0.16); + --btn-shadow: rgba(0, 0, 0, 0.16); + box-shadow: 0 2px 4px 0 var(--btn-shadow); + transition: box-shadow 0.3s ease; &:hover { - box-shadow: 0 4px 12px 0 rgba(0, 0, 0, 0.16); + box-shadow: 0 4px 12px 0 var(--btn-shadow);Also applies to: 292-298
Line range hint
1-424: Consider architectural improvements for scroll handling and footer integration.Given the PR's objective to add a common footer and optimize demo scrolling, consider these improvements:
The scroll listener cleanup could be more robust
The debounce timing might affect scroll smoothness
The component might need adaptation for the new footer integration
Enhance scroll listener cleanup:
const removeScrollListener = () => { - const docLayout = document.getElementById('doc-layout') || {} - docLayout.onscroll = null + const docLayout = document.getElementById('doc-layout') + if (docLayout?.onscroll) { + docLayout.onscroll = null + } }
- Consider adjusting the debounce timing:
-docLayout.onscroll = debounce(100, false, () => { +docLayout.onscroll = debounce(50, false, () => {
- Add resilience to footer integration:
const setScrollListener = () => { nextTick(() => { const docLayout = document.getElementById('doc-layout') + // Ensure footer exists before setting up scroll listener + if (!docLayout?.querySelector('footer')) { + console.warn('Footer not found, scroll behavior might be affected') + }examples/sites/public/static/css/design-common.css (4)
563-570: Consider adding width constraints for better layout control.While using percentage-based width is good for responsiveness, consider adding
min-widthandmax-widthconstraints to ensure optimal readability across different viewport sizes..tinyui-design-footer .footer-content { display: flex; margin: 0 auto; padding: 10vh 0 6.8vh; justify-content: center; width: 70%; + min-width: 320px; + max-width: 1440px; box-sizing: border-box; }
571-580: Replace viewport-based border width with fixed units.Using viewport units (
0.55vw) for border width can lead to inconsistent visual appearance across different screen sizes. Consider using fixed units for more predictable borders..tinyui-design-footer .footer-content .contact .group-code > img { width: 150px; - border: solid 0.55vw #f2f6f9; + border: solid 8px #f2f6f9; }
631-694: Consider using CSS custom properties for better maintainability.Using CSS custom properties (variables) for repeated values like colors, opacities, and spacing would improve maintainability and consistency.
+ :root { + --footer-border-color: #979797; + --footer-border-opacity: 0.28; + --footer-spacing-base: 4.7%; + } .footer-logo-container { position: relative; - padding-right: 4.7%; - margin-right: 4.7%; + padding-right: var(--footer-spacing-base); + margin-right: var(--footer-spacing-base); box-sizing: border-box; } .footer-logo-container::after { content: ''; position: absolute; display: block; height: 135px; - border-right: 1px solid #979797; - opacity: 0.28; + border-right: 1px solid var(--footer-border-color); + opacity: var(--footer-border-opacity); top: 60px; right: 0; }
695-707: Consider using URL variables for asset paths.The hardcoded background image path could break if assets are moved or the build process changes. Consider using a CSS variable or build-time replacement for the asset URL.
+ :root { + --icon-arrow-url: url(assets/icon-arrow.png); + } .icon-arrow { /* ... other properties ... */ - background: url(assets/icon-arrow.png) no-repeat; + background: var(--icon-arrow-url) no-repeat; background-size: 100% 100%; }examples/sites/src/views/components/components.vue (2)
Line range hint
1-496: Add error handling for footer renderingThe footer rendering relies on the global
TDCommonclass without any error handling. Consider wrapping the footer initialization in a try-catch block to handle potential failures gracefully.onMounted(() => { loadPage() - const common = new window.TDCommon(['#footer'], {}) - common.renderFooter() + try { + const common = new window.TDCommon(['#footer'], {}) + common.renderFooter() + } catch (error) { + console.error('Failed to render footer:', error) + } setScrollListener() })
Line range hint
1-496: Consider splitting the component into smaller, focused componentsThe current component handles multiple responsibilities including:
- Demo management
- API documentation
- Navigation
- Scroll handling
- Footer rendering
This makes the component complex and harder to maintain. Consider breaking it down into smaller, focused components:
DemoViewerApiDocumentationNavigationAnchorThis refactoring would:
- Improve code maintainability
- Make testing easier
- Reduce cognitive load when making changes
- Allow for better component reuse
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
⛔ Files ignored due to path filters (8)
examples/sites/public/static/images/footer/cli.svgis excluded by!**/*.svgexamples/sites/public/static/images/footer/code-open.pngis excluded by!**/*.pngexamples/sites/public/static/images/footer/footer-title-community.svgis excluded by!**/*.svgexamples/sites/public/static/images/footer/footer-title-help.svgis excluded by!**/*.svgexamples/sites/public/static/images/footer/footer-title-human.svgis excluded by!**/*.svgexamples/sites/public/static/images/footer/footer-title-resource.svgis excluded by!**/*.svgexamples/sites/public/static/images/footer/pro.svgis excluded by!**/*.svgexamples/sites/public/static/images/footer/theme.svgis excluded by!**/*.svg
📒 Files selected for processing (5)
examples/sites/public/static/css/design-common.css(1 hunks)examples/sites/public/static/js/design-common.js(2 hunks)examples/sites/src/views/components/components.vue(1 hunks)examples/sites/src/views/components/float-settings.vue(2 hunks)examples/sites/src/views/layout/layout.vue(0 hunks)
💤 Files with no reviewable changes (1)
- examples/sites/src/views/layout/layout.vue
🧰 Additional context used
🪛 Biome
examples/sites/public/static/css/design-common.css
[error] 779-779: Unexpected shorthand property margin after margin-left
(lint/suspicious/noShorthandPropertyOverrides)
🔇 Additional comments (6)
examples/sites/public/static/js/design-common.js (1)
78-122: Verify and secure external links.
The external links in the footer need additional security measures:
- All external links should be validated and maintained in a configuration file
- Consider implementing a link checker in the CI pipeline
Let's verify the external links and their security attributes:
examples/sites/public/static/css/design-common.css (4)
557-562: LGTM! Base footer styles are well-defined.
The base styles provide a clean foundation with appropriate font size, color contrast, and background color.
581-598: LGTM! Well-implemented grid layout with proper text handling.
The grid layout with auto columns provides good responsiveness, and the links have appropriate hover states and text overflow handling.
599-630: LGTM! Typography and spacing are well-defined.
The text and title styles maintain consistency with appropriate line heights and spacing.
708-820: LGTM! Comprehensive responsive design implementation.
The media queries provide a well-structured responsive design with appropriate breakpoints and layout adjustments for different screen sizes. The mobile-first approach with progressive enhancement is well implemented.
🧰 Tools
🪛 Biome
[error] 779-779: Unexpected shorthand property margin after margin-left
(lint/suspicious/noShorthandPropertyOverrides)
examples/sites/src/views/components/components.vue (1)
496-496: Consider potential race conditions with immediate scrolling
While reducing the timeout to 0ms improves responsiveness, ensure that the DOM elements are fully rendered before scrolling. The current implementation relies on nextTick and allDemoMounted, which should handle this, but it's worth verifying this behavior across different scenarios.
✅ Verification successful
Based on the search results, I can now verify the scroll timing concern. The codebase shows:
- The
scrollByHashfunction is used consistently across the codebase with various timing configurations - The function is used in conjunction with Vue's
nextTickand other DOM-ready checks - The component has proper checks for
allDemoMountedstate before scrolling - Similar scroll timing patterns (0ms timeout) are used in other components like
componentsDetail.vueandcomponentsDetailTab.vuewithout issues
Let me verify the DOM readiness checks by searching for the related code:
Let me check the complete implementation of the scroll handling to verify the safety of the timing change:
Let me get the rest of the scrollByHash implementation to see the complete flow:
Let me verify how this scroll function is invoked by checking its usage context:
Based on the comprehensive analysis of the codebase, I can now provide the final response:
The 0ms timeout is safe with the current implementation
The scroll timing change is safe because:
- The
scrollByHashis only called afterallDemoMounted()promise resolves, ensuring DOM readiness - The function has proper error handling for invalid hash values
- The scroll operation uses
behavior: 'smooth'for a controlled scroll animation - Similar 0ms timeout patterns are used successfully in other components (componentsDetail.vue, componentsDetailTab.vue)
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for any scroll-related issues or error handling in the codebase
# Look for scroll event handlers and timing-related code
rg -A 3 'scroll(To|By|Into)'
Length of output: 93915
Script:
#!/bin/bash
# Check for DOM readiness checks and allDemoMounted usage
rg -A 3 'allDemoMounted|nextTick.*scroll' examples/sites/src/views/components/components.vue
Length of output: 302
Script:
#!/bin/bash
# Get the complete scroll handling implementation
rg -A 15 'const scrollByHash' examples/sites/src/views/components/components.vue
Length of output: 606
Script:
#!/bin/bash
# Get the complete scrollByHash implementation with more context
rg -B 5 -A 25 'const scrollByHash' examples/sites/src/views/components/components.vue
Length of output: 885
Script:
#!/bin/bash
# Find where scrollByHash is called to understand the execution flow
rg -B 5 -A 5 'scrollByHash\(' examples/sites/src/views/components/components.vue
Length of output: 943
|
|
||
| <div class="footer-promotional-label">企业级设计体系</div> | ||
|
|
||
| <div class="footer-promotional-label">追求极致体验，多种设计方法融合</div> | ||
|
|
||
| <div class="footer-promotional-label">开箱即用的多语言组件库</div> | ||
|
|
||
| <div class="footer-logolist-container"> | ||
|
|
||
| <a href="/pro" class="footer-logo-link footer-logo-link-list"> | ||
| <img src="/static/images/footer/pro.svg" title="TinyPro"> | ||
| </a> | ||
|
|
||
| <a href="/designtheme/home" class="footer-logo-link footer-logo-link-list"> | ||
| <img src="/static/images/footer/theme.svg" title="TinyTheme"> | ||
| </a> | ||
|
|
||
| <a href="/tiny-cli/home" class="footer-logo-link footer-logo-link-list"> | ||
| <img src="/static/images/footer/cli.svg" title="TinyCLI"> | ||
| </a> | ||
|
|
||
| </div> | ||
| </div> | ||
|
|
||
| </div> | ||
|
|
||
| <div class="links"> | ||
|
|
||
| <div class="link-item"> | ||
| <div class="footer-title-container"> | ||
| <img src="/static/images/footer/footer-title-resource.svg" class="footer-logo-s"> | ||
| <span class="footer-title">相关资源</span> | ||
| <span class="icon-arrow"></span> | ||
| </div> | ||
| <div class="footer-text-list"> | ||
|
|
||
|
|
||
| <a href="https://angular.cn/" title="Angular - 中文网" target="_blank" rel="noopener noreferrer" class="list-item">Angular - 中文网</a> | ||
|
|
||
| <a href="https://cn.vuejs.org/" title="Vue - 中文网" target="_blank" rel="noopener noreferrer" class="list-item">Vue - 中文网</a> | ||
|
|
||
| <a href="https://ionic.io/ionicons" title="ionicons 图标库" target="_blank" rel="noopener noreferrer" class="list-item">ionicons 图标库</a> | ||
|
|
||
| </div> | ||
| </div> | ||
|
|
||
| <div class="link-item"> | ||
| <div class="footer-title-container"> | ||
| <img src="/static/images/footer/footer-title-community.svg" class="footer-logo-s"> | ||
| <span class="footer-title">社区</span> | ||
| <span class="icon-arrow"></span> | ||
| </div> | ||
| <div class="footer-text-list"> | ||
|
|
||
| <a href="https://mp.weixin.qq.com/s/3a6GaLY6hBGzV0nGzen2gw" title="公众号 - OpenTiny" target="_blank" rel="noopener noreferrer" class="list-item">公众号 - OpenTiny</a> | ||
|
|
||
| <a href="https://juejin.cn/user/3808325101432983" title="掘金 - OpenTiny 社区" target="_blank" rel="noopener noreferrer" class="list-item">掘金 - OpenTiny 社区</a> | ||
|
|
||
| <a href="https://www.zhihu.com/people/opentiny" title="知乎 - OpenTiny 社区" target="_blank" rel="noopener noreferrer" class="list-item">知乎 - OpenTiny 社区</a> | ||
|
|
||
| <a href="https://space.bilibili.com/15284299" title="B 站 - OpenTiny 社区" target="_blank" rel="noopener noreferrer" class="list-item">B 站 - OpenTiny 社区</a> | ||
|
|
||
| </div> | ||
| </div> | ||
|
|
||
| <div class="link-item"> | ||
| <div class="footer-title-container"> | ||
| <img src="/static/images/footer/footer-title-help.svg" class="footer-logo-s"> | ||
| <span class="footer-title">帮助</span> | ||
| <span class="icon-arrow"></span> | ||
| </div> | ||
| <div class="footer-text-list"> | ||
|
|
||
| <a href="https://github.com/opentiny" title="GitHub" target="_blank" rel="noopener noreferrer" class="list-item">GitHub</a> | ||
|
|
||
| <a href="https://gitee.com/opentiny" title="Gitee" target="_blank" rel="noopener noreferrer" class="list-item">Gitee</a> | ||
|
|
||
| <a href="/tiny-vue/zh-CN/os-theme/docs/changelog" title="更新日志" target="_blank" rel="noopener noreferrer" class="list-item">更新日志</a> | ||
|
|
||
| <a href="https://github.com/opentiny/tiny-vue/issues" title="讨论区" target="_blank" rel="noopener noreferrer" class="list-item">讨论区</a> | ||
|
|
||
| </div> | ||
| </div> | ||
|
|
||
| <div class="contact"> | ||
| <div> | ||
| <img src="/static/images/footer/footer-title-human.svg" class="footer-logo-s"> | ||
| <span class="footer-title">联系我们</span> | ||
| </div> | ||
| <div class="group-code"> | ||
| <img src="/static/images/footer/code-open.png"> | ||
| </div> | ||
| </div> | ||
| </div> | ||
| </div> | ||
| <div class="copyright"> | ||
| <p>Copyright © Huawei Technologies Co., Ltd. 2023. All rights reserved.</p> | ||
| <p>粤ICP备2022156931号-1</p> | ||
| </div> | ||
| </div>` |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Enhance accessibility, security, and maintainability of the footer template.
Several improvements could be made to the footer template:
- Add ARIA labels and roles for better accessibility
- Enhance security for external links
- Consider internationalizing the text content
- Optimize image loading
- Consider modularizing the template
Here's a sample of the recommended changes:
const footerHtml = `<div class="tinyui-design-footer">
- <div class="footer-content">
+ <div class="footer-content" role="contentinfo">
<div class="footer-logo-container">
- <a href="/" class="footer-logo-link">
+ <a href="/" class="footer-logo-link" aria-label="OpenTiny Home">
- <img src="/static/images/opentiny-logo.svg" title="OpenTiny" class="footer-logo">
+ <img src="/static/images/opentiny-logo.svg" alt="OpenTiny Logo" class="footer-logo" loading="lazy">
<span class="footer-logo-title">OpenTiny</span>
</a>Consider extracting the template into smaller, reusable components:
const footerSections = {
logo: createLogoSection(),
resources: createResourcesSection(),
community: createCommunitySection(),
help: createHelpSection(),
contact: createContactSection()
};
const footerHtml = createFooterTemplate(footerSections);| renderFooter() { | ||
| // 空方法，用于给官网执行，解决报错 | ||
| document.getElementById('footer').innerHTML = footerHtml | ||
| } |
There was a problem hiding this comment.
Add error handling and security measures to footer rendering.
The current implementation has several potential issues:
- No error handling if the footer element doesn't exist
- Direct innerHTML assignment without sanitization
- No cleanup of previous content
Consider implementing these improvements:
renderFooter() {
- document.getElementById('footer').innerHTML = footerHtml
+ const footerElement = document.getElementById('footer')
+ if (!footerElement) {
+ console.error('Footer element not found')
+ return
+ }
+ // Clean up existing content
+ while (footerElement.firstChild) {
+ footerElement.removeChild(footerElement.firstChild)
+ }
+ // Use DOMPurify or similar library to sanitize HTML
+ footerElement.innerHTML = DOMPurify.sanitize(footerHtml)
}Committable suggestion skipped: line range outside the PR's diff.
| const { scrollTop: layoutScrollTop } = docLayout | ||
| const headerHeight = docLayout.querySelector('header')?.clientHeight || 0 | ||
| const footerHeight = docLayout.querySelector('#footer')?.clientHeight || 0 | ||
| const footerTop = footer.getBoundingClientRect().top | ||
| const navHeight = nav?.clientHeight || 0 | ||
| const currSettingsBottom = parseInt(window.getComputedStyle(floatSettings.value).getPropertyValue('bottom')) | ||
| const footerVisibleHeight = footerHeight - (layoutScrollHeight - layoutScrollTop - layoutHeight) | ||
|
|
||
| state.showBackTop = layoutScrollTop > 500 | ||
| state.showSettings = footerTop - navHeight - headerHeight > 220 | ||
|
|
||
| if (!state.initBottomVal) { | ||
| state.initBottomVal = currSettingsBottom | ||
| } |
There was a problem hiding this comment.
💡 Codebase verification
After analyzing the complete file, I can now provide the final response:
Fix undefined footer variable and update scroll calculations
The showSettings state is actually unused in the template and the code has an undefined footer variable. Here's what needs to be fixed:
- The
footervariable is undefined but used in the scroll calculations. This will cause runtime errors. - The
showSettingsstate is being set but never used in the template or styles, making it dead code.
- const footerTop = footer.getBoundingClientRect().top
+ const footer = docLayout.querySelector('footer')
+ const footerTop = footer?.getBoundingClientRect().top || 0
- state.showSettings = footerTop - navHeight - headerHeight > 220🔗 Analysis chain
Fix undefined variable and potential scroll calculation issues.
There are several issues in the scroll handling logic:
- The
footervariable is undefined but used infooterTopcalculation - The
showSettingsstate is set but not used in the template
Fix the undefined variable by properly selecting the footer element:
- const footerTop = footer.getBoundingClientRect().top
+ const footer = docLayout.querySelector('footer')
+ const footerTop = footer?.getBoundingClientRect().top || 0Let's verify the usage of showSettings state:
Let me gather more information about the usage of showSettings in the template and any potential style bindings.
Let me try a different approach to search for the template content and footer-related code.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for showSettings usage in the template
rg 'showSettings' examples/sites/src/views/components/float-settings.vue
Length of output: 148
Script:
#!/bin/bash
# Search for style bindings or v-show directives in the template section
ast-grep --pattern '<template>
$$$
</template>' examples/sites/src/views/components/float-settings.vue
# Also check for footer element selection
rg -B2 -A2 'querySelector.*footer' examples/sites/src/views/components/float-settings.vue
Length of output: 198
Script:
#!/bin/bash
# Get the entire content of the file to analyze template and script sections
cat examples/sites/src/views/components/float-settings.vue
# Specifically look for footer-related code
rg -l 'footer' examples/sites/src/views/components/float-settings.vue
Length of output: 10897
| margin-left: 0; | ||
| margin: 30px auto 12px; | ||
| } |
There was a problem hiding this comment.
Fix the margin property override.
The margin shorthand property is overriding the previously set margin-left: 0. This can lead to unexpected behavior. Combine the margin declarations or use specific margin properties.
.tinyui-design-footer .footer-content .contact .group-code {
- margin-left: 0;
- margin: 30px auto 12px;
+ margin: 30px auto 12px 0;
}📝 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.
| margin-left: 0; | |
| margin: 30px auto 12px; | |
| } | |
| margin: 30px auto 12px 0; | |
| } |
🧰 Tools
🪛 Biome
[error] 779-779: Unexpected shorthand property margin after margin-left
(lint/suspicious/noShorthandPropertyOverrides)
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