feat(anchor): [anchor] add top-offset props #2388
Conversation
WalkthroughThe pull request introduces modifications to the anchor component across multiple files. A new property, Changes
Possibly related PRs
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 (
|
![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)
packages/vue/src/anchor/src/index.ts (1)
36-38: Consider adding JSDoc documentation and value validation.To improve developer experience and prevent potential issues:
- Add JSDoc comments describing the purpose and usage of topOffset
- Consider adding validation for negative values if they're not supported
+ /** + * Specifies the top offset for anchor positioning + * @default 0 + */ topOffset: { type: Number, - default: 0 + default: 0, + validator: (value: number) => value >= 0 }packages/vue/src/anchor/src/pc.vue (3)
10-10: Add TypeScript type definition and documentation for the topOffset prop.While the prop is added correctly, it would benefit from proper TypeScript typing and documentation to improve maintainability and developer experience.
Apply this diff to add type information and documentation:
- props: [...props, 'isAffix', 'links', 'containerId', 'markClass', 'type', 'topOffset'], + props: { + ...props, + isAffix: Boolean, + links: Array, + containerId: String, + markClass: String, + type: String, + /** + * The offset from the top of the viewport when calculating anchor positions + * @default 0 + */ + topOffset: { + type: Number, + default: 0 + } + },
11-11: Translate deprecation notice to English.The deprecation notice contains Chinese characters. For better international developer experience, it should be in English.
- emits: ['linkClick', 'onChange', 'change'], // deprecated v3.12.0废弃，v3.17.0移除onChange 事件 + emits: ['linkClick', 'onChange', 'change'], // 'onChange' is deprecated since v3.12.0 and will be removed in v3.17.0
Line range hint
16-54: Consider extracting render functions for better maintainability.The render logic contains nested functions with complex JSX. Consider extracting these into separate methods or components for better maintainability and readability.
Here's a suggested approach to refactor the render methods:
// Extract link rendering logic const AnchorLink = ({ item, currentLink, linkClick, anchorClass }) => ( <div class={`${anchorClass}-link`} key={item.key}> <a href={item.link} class={[ `${anchorClass}-link-title`, currentLink === item.link && `${anchorClass}-link-title--active` ]} onClick={(e) => linkClick(e, item)} ref={item.link} v-auto-tip> {item.title} </a> {item.children ? <AnchorLinks links={item.children} {...{ currentLink, linkClick, anchorClass }} /> : null} </div> ); // Extract links list rendering const AnchorLinks = ({ links, currentLink, linkClick, anchorClass }) => ( Array.isArray(links) ? links.map(item => ( <AnchorLink item={item} {...{ currentLink, linkClick, anchorClass }} /> )) : null );packages/renderless/src/anchor/index.ts (1)
Line range hint
171-235: Consider documenting the topOffset behavior.The implementation of
topOffsetis well-integrated into both the intersection observer and scroll calculations. However, consider adding JSDoc comments to document:
- The purpose and expected behavior of
topOffset- The units expected (pixels)
- How it affects the anchor positioning
Add documentation above the props interface:
/** * @property {number} topOffset - The distance in pixels to offset the anchor scroll position from the top. * This affects both the intersection observer's root margin and the final scroll position. * Must be a non-negative number. Defaults to 0. */
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (3)
- packages/renderless/src/anchor/index.ts (4 hunks)
- packages/vue/src/anchor/src/index.ts (1 hunks)
- packages/vue/src/anchor/src/pc.vue (1 hunks)
🔇 Additional comments (3)
packages/vue/src/anchor/src/index.ts (1)
36-38: LGTM! The topOffset property is correctly implemented.The property is well-integrated into the existing anchorProps object with appropriate type and default value.
packages/vue/src/anchor/src/pc.vue (1)
Line range hint
10-14: Verify the topOffset prop usage in the component.The
topOffsetprop is added but not visibly used in the render function. Let's verify its integration with the renderless logic.✅ Verification successful
Based on the results, I can see that
topOffsetis properly integrated in the renderless implementation. Let me verify the test coverage with a different approach.This will help us understand if there's adequate test coverage for the
topOffsetfunctionality, even if it's not explicitly mentioned in the test file names.
Let me try one more verification to check the implementation details more thoroughly.
This will help us understand the complete context of how
topOffsetis integrated into the component's logic.
Based on the implementation code, I can now provide a final response:
The
topOffsetprop is properly integrated and utilized in the componentThe
topOffsetprop is used in two critical places in the renderless implementation:
- In the
onItersectionObserverfunction to set up therootMarginfor intersection observation- In the
linkClickfunction to correctly calculate the scroll position, specifically addressing the issue of horizontal anchor scrolling to the topThe implementation shows that the prop serves its intended purpose by:
- Adjusting the intersection observer's root margin to account for the offset:
rootMargin = topOffset ? ${-topOffset}px 0px 0px 0px : ''- Properly calculating scroll positions in the click handler by subtracting the
topOffsetvalue🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify the usage of topOffset prop in the renderless implementation # and any related test coverage # Check implementation echo "Checking renderless implementation..." rg -A 10 "topOffset" "packages/renderless/src/anchor/" # Check test coverage echo "Checking test coverage..." rg -A 10 "topOffset" "packages/vue/src/anchor/__tests__/"Length of output: 1743
Script:
#!/bin/bash # Let's check for test files and their content more broadly echo "Checking test files..." fd -e spec.ts -e test.ts . packages/vue/src/anchor/__tests__/ echo "---" echo "Checking content of test files..." fd -e spec.ts -e test.ts . packages/vue/src/anchor/__tests__/ -x cat {}Length of output: 286
Script:
#!/bin/bash # Let's examine the full renderless implementation to understand the topOffset usage context echo "Checking full renderless implementation..." cat packages/renderless/src/anchor/index.ts # Also check the interface definitions echo "---" echo "Checking interface definitions..." rg -A 5 "interface.*Anchor" packages/vue/src/anchor/src/Length of output: 8196
packages/renderless/src/anchor/index.ts (1)
Line range hint
197-209: LGTM! Improved intersection detection logic.The simplified intersection condition and updated observer options provide more precise anchor detection. The use of multiple threshold values (0, 0.25, 0.5, 1) allows for smoother transitions.
| @@ -168,11 +168,11 @@ export const onItersectionObserver = | |||
| ({ state, props, api, vm, emit }: Pick<IAnchorRenderlessParams, 'state' | 'props' | 'api' | 'vm' | 'emit'>) => | |||
| () => { | |||
| const { expandLink, scrollContainer } = state | |||
| const { topOffset } = props | |||
There was a problem hiding this comment.
Add type checking and validation for topOffset.
The topOffset property is used without type checking or validation. Consider adding runtime checks to ensure it's a non-negative number.
Add validation before using topOffset:
- const { topOffset } = props
+ const topOffset = Math.max(0, Number(props.topOffset) || 0)Also applies to: 173-173
| const top = | ||
| linkEl?.getBoundingClientRect().top - | ||
| scrollContainer.getBoundingClientRect().top + | ||
| scrollContainer.scrollTop - | ||
| props.topOffset // 修复横向锚点无法滚动到顶部 |
There was a problem hiding this comment.
Add null safety and consider cross-browser compatibility.
The scroll position calculation looks correct but has the following concerns:
- Missing null check for
linkElbefore accessing getBoundingClientRect - The
scrollTowithsmoothbehavior isn't supported in all browsers
Add null safety and fallback:
- const top =
- linkEl?.getBoundingClientRect().top -
- scrollContainer.getBoundingClientRect().top +
- scrollContainer.scrollTop -
- props.topOffset
- const param = { top, left: 0, behavior: 'smooth' } as ScrollToOptions
- scrollContainer?.scrollTo(param)
+ if (linkEl) {
+ const top =
+ linkEl.getBoundingClientRect().top -
+ scrollContainer.getBoundingClientRect().top +
+ scrollContainer.scrollTop -
+ (props.topOffset || 0)
+ try {
+ scrollContainer.scrollTo({ top, left: 0, behavior: 'smooth' })
+ } catch (e) {
+ // Fallback for browsers that don't support smooth scrolling
+ scrollContainer.scrollTop = top
+ }
+ }📝 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 top = | |
| linkEl?.getBoundingClientRect().top - | |
| scrollContainer.getBoundingClientRect().top + | |
| scrollContainer.scrollTop - | |
| props.topOffset // 修复横向锚点无法滚动到顶部 | |
| if (linkEl) { | |
| const top = | |
| linkEl.getBoundingClientRect().top - | |
| scrollContainer.getBoundingClientRect().top + | |
| scrollContainer.scrollTop - | |
| (props.topOffset || 0) | |
| try { | |
| scrollContainer.scrollTo({ top, left: 0, behavior: 'smooth' }) | |
| } catch (e) { | |
| // Fallback for browsers that don't support smooth scrolling | |
| scrollContainer.scrollTop = top | |
| } | |
| } |
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
topOffsetproperty for the anchor component, allowing for dynamic adjustments to scrolling behavior.Bug Fixes
Documentation
topOffsetproperty across various components.