fix(pages): Features nav scrolls to #features section instead of navigating to / - #537
fix(pages): Features nav scrolls to #features section instead of navigating to /#537Sunil56224972 wants to merge 5 commits into
Conversation
Closes alibaba#495 The Features nav tab pointed to '/' which was indistinguishable from clicking the logo. Change it to '/#features' so it scrolls to the features section on the landing page. - Add handleNavClick that splits hash routes and scrolls to the target element when already on the correct page - Add useEffect to scroll to hash target after cross-page navigation - Update isActive logic to highlight Features only when the hash matches
|
🔍 OpenCodeReview found 5 issue(s) in this PR.
|
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
| { value: 'zh', label: 'ä¸æ–‡' }, | ||
| { value: 'ja', label: '日本語' }, |
There was a problem hiding this comment.
Character encoding corruption: The Chinese and Japanese labels are stored as mojibake (ä¸æ–‡ instead of 中文, 日本語 instead of 日本語). This will render as garbled text in the browser, making the language selector unusable for non-English users. The same issue appears on line 182 for the mobile language button text. Please ensure the file is saved with UTF-8 encoding and re-commit with correct characters.
| const timer = setTimeout(() => { | ||
| const el = document.getElementById(id); | ||
| if (el) { | ||
| el.scrollIntoView({ behavior: 'smooth' }); | ||
| } | ||
| }, 100); |
There was a problem hiding this comment.
Fragile hardcoded delay: The 100ms setTimeout is an arbitrary magic number that may not be sufficient on slower devices or pages with heavier rendering. Consider using requestAnimationFrame (possibly double-rAF to wait for paint) or a MutationObserver/ResizeObserver to detect when the target element is actually rendered, rather than relying on a fixed timeout. Also note that this effect runs whenever currentPath changes even if currentHash hasn't changed, which could cause unnecessary scroll attempts.
| fontFamily: language === 'ja' ? "'Hiragino Sans', sans-serif" : "'PingFang SC', -apple-system, sans-serif", | ||
| }}> | ||
| {language === 'en' ? 'En' : language === 'zh' ? '中' : 'あ'} | ||
| {language === 'en' ? 'En' : language === 'zh' ? 'ä¸' : 'ã‚'} |
There was a problem hiding this comment.
Nested ternary expression: This violates the code quality rule prohibiting nested ternaries. Consider using a lookup object for better readability and maintainability:
const LANG_ABBREVIATIONS: Record<Language, string> = { en: 'En', zh: '中', ja: 'あ' };Then use {LANG_ABBREVIATIONS[language]} here.
| <div id="features"> | ||
| <HighlightsSection /> | ||
| </div> |
There was a problem hiding this comment.
Potential race condition with ScrollToTop: When navigating from another page (e.g., /benchmark) to /#features, the ScrollToTop component in App.tsx will first scroll the window to top (triggered by pathname change to /). Then this useEffect fires after a 100ms delay to scroll to the #features element. This causes a visible double-scroll effect (jump to top, then smooth scroll down).
Consider either:
- Making
ScrollToTopskip scrolling when there's a hash present, or - Increasing the delay / using
requestAnimationFrameto ensure the scroll-to-top has completed before scrolling to the anchor, or - Using an immediate (non-smooth) scroll in this useEffect to avoid the visual glitch.
| <div id="features"> | ||
| <HighlightsSection /> | ||
| </div> |
There was a problem hiding this comment.
UX regression: No active tab on root path /: Previously, the Features tab had path: '/' and was marked active whenever currentPath === '/'. Now it requires both currentPath === '/' AND currentHash === '#features'. This means when a user visits / directly (without the hash), no nav tab will be highlighted as active.
Consider falling back to treating / without a hash as the features tab being active, e.g.:
const isActive = tab.path === '/#features'
? currentPath === '/' && (currentHash === '#features' || currentHash === '')
: currentPath.startsWith(tab.path);
lizhengfeng101
left a comment
lizhengfeng101
left a comment
There was a problem hiding this comment.
Character encoding corruption
- Fix UTF-8 encoding corruption (mojibake in CJK labels) - Replace fragile 100ms setTimeout with double-rAF - Extract nested ternary into LANG_ABBREVIATIONS lookup - Fix useEffect to depend only on currentHash (not currentPath) - Fall back to Features tab active on '/' without hash
Prevents double-scroll effect (jump-to-top then smooth-scroll-down) when navigating from another page to /#features.
|
Addressed all 5 review comments. Here's what changed: 1. Encoding corruption — Rebuilt all files from upstream UTF-8 source. CJK characters are now correct. 2. Fragile 100ms setTimeout — Replaced with double 3. Nested ternary — Extracted into a 4. ScrollToTop race condition — Updated 5. No active tab on Files changed:
|
|
Thank you for your contribution! Unfortunately, this change has become outdated — we've already merged an earlier PR that addresses the same issue. We appreciate the effort and would love to see you contribute again in the future. Feel free to pick up any open issues or propose new improvements! |
2 participants
Summary
Fixes #495
The Features nav tab in the Navbar navigated to
/which is identical to clicking the logo ? making it indistinguishable from Home.Changes
pages/src/components/Navbar.tsx/to/#featureshandleNavClickcallback that handles hash-based navigation:#featuresanchoruseEffectto scroll to hash target after cross-page navigation (with 100ms delay for render)isActivelogic: Features tab is highlighted only whencurrentPath === '/'ANDcurrentHash === '#features'(no longer highlighted just from being on the home page)pages/src/pages/FeaturesPage.tsxHighlightsSectionin a<div id="features">to provide the scroll anchor targetAcceptance Criteria (from #495)