refactor: [anchor] rename topOffset and reverse judge condition #2394
Conversation
WalkthroughThe changes in this pull request primarily involve renaming state and property variables related to anchor link management within the 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 (
|
|
[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: 0
🧹 Outside diff range and nitpick comments (3)
packages/vue/src/anchor/src/index.ts (1)
36-39: Add JSDoc for the offsetTop propertyConsider adding JSDoc documentation to describe the purpose and usage of this property.
+ /** + * The top offset value for the anchor positioning + * @default 0 + */ offsetTop: { type: Number, default: 0 }packages/vue/src/anchor/src/pc.vue (1)
11-11: Consider removing the deprecatedonChangeevent.The comment indicates that the
onChangeevent was deprecated in v3.12.0 and scheduled for removal in v3.17.0. If we're past v3.17.0, this comment and the deprecated event should be removed.- emits: ['linkClick', 'onChange', 'change'], // deprecated v3.12.0废弃，v3.17.0移除onChange 事件 + emits: ['linkClick', 'change'],packages/renderless/src/anchor/index.ts (1)
242-242: LGTM! Consider enhancing the inline comment.The property rename from
topOffsettooffsetTopis consistent with other changes. However, the comment could be more descriptive.Consider updating the comment to better explain the fix:
- props.offsetTop // 修复横向锚点无法滚动到顶部 + props.offsetTop // Subtracts the offset to ensure proper scroll position for both vertical and horizontal anchors
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (5)
- packages/renderless/src/anchor/index.ts (4 hunks)
- packages/renderless/src/anchor/vue.ts (1 hunks)
- packages/renderless/types/anchor.type.ts (1 hunks)
- packages/vue/src/anchor/src/index.ts (1 hunks)
- packages/vue/src/anchor/src/pc.vue (1 hunks)
🔇 Additional comments (8)
packages/vue/src/anchor/src/index.ts (1)
36-39: Verify impact of property rename fromtopOffsettooffsetTopWhile the rename improves naming consistency, changing prop names is typically considered a breaking change. This contradicts the PR's "no breaking changes" statement.
Let's check for existing usages:
Consider:
- Adding a deprecation warning for the old prop name
- Supporting both prop names temporarily
- Documenting this change in the migration guide
packages/renderless/types/anchor.type.ts (2)
28-28: Consider updating documentation.Since this is a public interface change, ensure that any associated documentation or API references are updated to reflect the new property name.
Let's check for documentation files that might need updates:
#!/bin/bash # Description: Look for documentation files that might reference the old property name echo "Checking documentation files for references to offsetTop..." fd -e md -e mdx | xargs rg "offsetTop"
28-28: Verify the rename impact across the codebase.The rename from
offsetToptochildOffsetTopin theIAnchorStateinterface appears to be for better clarity, reflecting its actual usage context. However, since this is an exported interface, we should verify that all consumers have been updated.Let's verify the rename has been consistently applied:
✅ Verification successful
Property rename is correctly implemented and consistent
The rename from
offsetToptochildOffsetTophas been properly implemented across the anchor component's codebase:
- The property is defined in the state interface (
IAnchorState)- It's used consistently in the implementation files:
packages/renderless/src/anchor/index.ts- Used insetChildOffsetTopfunctionpackages/renderless/src/anchor/vue.ts- Defined in the state object- The property is internal to the component's state and not exposed in the public API (props)
- All references to this state property have been updated
The rename is a safe, non-breaking change as it only affects the internal state management of the anchor component.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check for any remaining references to the old property name # and verify all usages have been updated to the new name. echo "Checking for any remaining references to state.offsetTop..." rg -l "state\.offsetTop" echo "Verifying all references now use state.childOffsetTop..." rg -l "state\.childOffsetTop" # Also check for type references echo "Checking for any TypeScript interface property definitions still using offsetTop..." ast-grep --pattern 'interface $_ { $$$ offsetTop: $_ $$$ }'Length of output: 262
Script:
#!/bin/bash # Let's try a more focused search approach # Search for any remaining references to offsetTop echo "=== Searching for 'offsetTop' references ===" rg "offsetTop" -l # Search for new childOffsetTop references echo -e "\n=== Searching for 'childOffsetTop' references ===" rg "childOffsetTop" -l # Search specifically in TypeScript files echo -e "\n=== Searching in TypeScript files for interface definitions ===" fd -e ts -e tsx --exec grep -l "offsetTop\|childOffsetTop" {}Length of output: 2705
Script:
#!/bin/bash # Let's examine the actual usage context in anchor-related files echo "=== Checking anchor implementation files ===" echo -e "\nContent of packages/renderless/src/anchor/index.ts:" cat packages/renderless/src/anchor/index.ts echo -e "\nContent of packages/renderless/src/anchor/vue.ts:" cat packages/renderless/src/anchor/vue.ts echo -e "\nContent of packages/vue/src/anchor/src/index.ts:" cat packages/vue/src/anchor/src/index.ts echo -e "\nContent of packages/vue/src/anchor/src/pc.vue:" cat packages/vue/src/anchor/src/pc.vueLength of output: 14353
packages/vue/src/anchor/src/pc.vue (2)
Line range hint
7-54: Implementation looks good!The component implementation is well-structured with:
- Clear separation of concerns using the renderless pattern
- Type-safe implementation with proper TypeScript types
- Clean and recursive rendering logic for nested links
- Proper handling of different anchor types
10-10: Verify the property rename direction across the codebase.There appears to be an inconsistency in the property naming across different parts of the codebase. Let's verify the correct direction of the rename and ensure consistency.
✅ Verification successful
Let me try a different approach with ripgrep since the
--typeflag didn't work as expected.
Based on the git history and code search results, I can now provide a definitive response about the property rename:
Property rename is correctly implemented from
topOffsettooffsetTopThe git history shows a clear commit (636884e) that explicitly renames the prop from
topOffsettooffsetTop. The code search also confirms thatoffsetTopis now consistently used across the codebase, particularly in the anchor-related functionality. This rename aligns with the PR objectives and maintains consistency with the component's implementation.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check the usage of both property names across the codebase # to determine the correct direction of the rename and ensure consistency echo "Checking for 'offsetTop' usage:" rg --type vue --type typescript 'offsetTop' -B 2 -A 2 echo -e "\nChecking for 'topOffset' usage:" rg --type vue --type typescript 'topOffset' -B 2 -A 2 # Check specifically in anchor-related files echo -e "\nChecking anchor-specific files:" fd -e ts -e vue anchorLength of output: 890
Script:
#!/bin/bash # Search for both property names in all files, focusing on the actual implementation echo "Searching for offsetTop usage:" rg 'offsetTop' -B 2 -A 2 echo -e "\nSearching for topOffset usage:" rg 'topOffset' -B 2 -A 2 # Also check the git history for this specific file to see the rename direction echo -e "\nChecking git history for the specific file:" git log -p packages/vue/src/anchor/src/pc.vueLength of output: 43616
packages/renderless/src/anchor/vue.ts (1)
57-57: Improved naming clarity withchildOffsetTop.The rename from
offsetToptochildOffsetTopprovides better clarity about the property's purpose in relation to child elements.Let's verify consistent renaming across the codebase:
packages/renderless/src/anchor/index.ts (2)
130-130: LGTM! Improved variable naming.The rename from
offsetToptochildOffsetTopbetter describes the variable's purpose, making the code more self-documenting.
170-177: LGTM! Enhanced intersection detection logic.The modified condition provides more precise intersection detection by:
- Properly accounting for container position
- Including both child and parent offsets
- Handling boundary cases with the <= operator
Let's verify the intersection logic works correctly with various offset combinations:
Also applies to: 199-203
✅ Verification successful
Based on the code analysis, I can now generate the final response:
Verified: The enhanced intersection detection logic is correct and well-implemented
The changes improve the accuracy of intersection detection by:
- Properly calculating the scroll start position using
scrollStartTop = top + childOffsetTop + offsetTop- Using the container's bounding rect to get the correct top position
- Maintaining state through
observerLinksfor proper tracking- Correctly handling hash changes and scroll states
The implementation ensures proper anchor positioning and scrolling behavior while respecting the configured offset values.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for tests that verify intersection observer behavior rg -l "IntersectionObserver.*offsetTop" --type tsLength of output: 51
Script:
#!/bin/bash # Let's try a broader search for intersection observer tests rg -l "IntersectionObserver" --type test rg -l "IntersectionObserver" "**/*test*" --type ts # Also search for anchor-related tests that might verify this behavior rg -l "anchor.*test" --type test rg -l "anchor.*spec" --type test # Look for actual usage of these offset calculations rg "scrollStartTop.*childOffsetTop.*offsetTop" --type ts -C 3Length of output: 965
Script:
#!/bin/bash # Let's try different search patterns to find tests fd -e test.ts -e spec.ts # Search for the actual usage of IntersectionObserver in the codebase rg "IntersectionObserver" -C 5 --type ts # Look for anchor-related test files with any extension fd "anchor" -e test.ts -e spec.ts -e test.js -e spec.js # Search for offset-related logic in anchor component rg "offsetTop|childOffsetTop" --type ts -C 3 packages/renderless/src/anchor/Length of output: 68964
Script:
#!/bin/bash # Let's examine the intersection observer logic more closely rg "observerLinks|currentLink" -C 5 packages/renderless/src/anchor/ # Look for any tests related to anchor scrolling behavior rg -l "anchor.*scroll" examples/sites/demos/pc/app/anchor/ # Check the anchor component's props and state definitions ast-grep --pattern 'interface $_ { $$$ offsetTop?: $_ $$$ }'Length of output: 8015
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (2)
packages/vue/src/anchor/src/pc.vue (1)
10-10: Document the breaking change in prop name.The rename from
topOffsettooffsetTopis a breaking change that should be documented in the changelog or migration guide.Consider adding a comment above the props declaration to indicate the change:
+ // @breaking-change topOffset prop has been renamed to offsetTop for better DOM terminology alignment props: [...props, 'isAffix', 'links', 'containerId', 'markClass', 'type', 'offsetTop'],packages/renderless/src/anchor/index.ts (1)
242-242: Consider moving the fix documentation to JSDocWhile the change correctly uses the renamed prop, the inline comment about fixing horizontal anchor scrolling would be more valuable as a JSDoc comment above the function.
Consider this documentation improvement:
+/** + * Handles anchor link clicks and scrolling + * @param {Event} e - Click event + * @param {IAnchorLinkItem} item - Anchor item + * @note The offsetTop adjustment ensures correct scrolling for both vertical and horizontal anchors + */ export const linkClick = ...
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (5)
- packages/renderless/src/anchor/index.ts (4 hunks)
- packages/renderless/src/anchor/vue.ts (1 hunks)
- packages/renderless/types/anchor.type.ts (1 hunks)
- packages/vue/src/anchor/src/index.ts (1 hunks)
- packages/vue/src/anchor/src/pc.vue (1 hunks)
🔇 Additional comments (10)
packages/vue/src/anchor/src/index.ts (1)
36-39: Consider handling this breaking change more gracefully.While renaming
topOffsettooffsetTopimproves naming consistency, this change could break existing implementations that use thetopOffsetprop.Consider:
- Supporting both prop names temporarily with
topOffsetmarked as deprecated- Adding proper documentation for this breaking change
Here's a script to check for existing usage:
packages/renderless/types/anchor.type.ts (2)
28-28: LGTM! The new property name is more descriptive.The rename from
offsetToptochildOffsetTopbetter describes the purpose of this property, making the code more self-documenting.
28-28: Verify consistent property renaming across implementations.Let's ensure all usages of this state property have been updated to use the new name.
✅ Verification successful
Property
childOffsetTopis used consistently in anchor componentBased on the search results, the property
childOffsetTopis correctly and consistently used across the anchor component implementation:
- Type definition in
packages/renderless/types/anchor.type.ts- Initial state in
packages/renderless/src/anchor/vue.ts- Usage in scroll calculations in
packages/renderless/src/anchor/index.tsThe occurrences of
offsetTopfound in the codebase are unrelated to this property as they belong to different components or represent DOM element properties.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify that all references to the old property name have been updated # Test 1: Search for any remaining references to 'offsetTop' # Expect: No references in anchor-related files except in comments echo "Checking for remaining 'offsetTop' references..." rg -g '!*.md' -g '!*.json' 'offsetTop' packages/ # Test 2: Verify the new property name is used consistently # Expect: References to 'childOffsetTop' in anchor component files echo "Checking usage of new 'childOffsetTop' property..." rg 'childOffsetTop' packages/Length of output: 9699
packages/vue/src/anchor/src/pc.vue (1)
10-10: Prop rename improves naming consistency but requires attention.The rename from
topOffsettooffsetTopbetter aligns with DOM terminology and improves code clarity.Let's verify the impact of this change:
✅ Verification successful
Prop rename is safe and properly implemented
The verification shows that:
- No occurrences of
topOffsetexist in the codebase, indicating the rename was complete- The new
offsetTopprop is properly used in the anchor component's implementation (packages/vue/src/anchor/src/index.ts and packages/vue/src/anchor/index.ts)- The change is consistent with the component's internal logic where
offsetTopis used for scroll calculations and observer margins🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for any remaining usage of topOffset in the codebase rg "topOffset" --type vue --type ts --type jsLength of output: 73
Script:
#!/bin/bash # Search for any remaining usage of topOffset in all files rg "topOffset" # Also search for offsetTop to understand the full scope of changes rg "offsetTop" # List all vue files in the anchor directory to understand the component structure fd -e vue -p "anchor"Length of output: 10268
packages/renderless/src/anchor/vue.ts (2)
Line range hint
1-96: Excellent code organization and practices!The overall structure of this renderless component demonstrates several good practices:
- Strong typing with TypeScript
- Clear separation of concerns with composable functions
- Proper reactive handling with watch effects
- Clean API composition pattern
57-57: LGTM! Verify related component usage.The rename from
offsetToptochildOffsetTopin the state object is consistent with the broader refactoring effort described in the PR. The new name better reflects its purpose in managing child component offsets.Let's verify the consistency of this rename across the codebase:
packages/renderless/src/anchor/index.ts (4)
130-130: LGTM: Consistent variable renamingThe rename from
topOffsettochildOffsetTopin the state improves clarity by better describing its purpose - storing the offset of the first child anchor link.
170-173: LGTM: Consistent prop usageThe changes correctly use the renamed props and state variables while maintaining the same rootMargin calculation logic.
176-177: LGTM: Improved intersection check readabilityThe changes improve the code in two ways:
- Consistent use of renamed variables in scrollStartTop calculation
- More intuitive intersection condition that's easier to understand
Also applies to: 199-203
Line range hint
1-246: Verify consistent variable renaming across the codebaseLet's ensure all instances of the old variable names have been updated consistently.
✅ Verification successful
No variable renaming inconsistencies found
Based on the search results:
- No instances of the old variable name
topOffsetwere found in the codebase- The new variables
offsetTopandchildOffsetTopare used consistently in the anchor component and other components where they represent legitimate, separate concepts- The changes in the anchor component are self-contained and properly implemented
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for any remaining instances of the old variable names echo "Checking for any remaining instances of 'topOffset'..." rg "topOffset" --type ts --type vue echo "Verifying consistent usage of new 'offsetTop' and 'childOffsetTop'..." rg "offsetTop|childOffsetTop" --type ts --type vueLength of output: 430
Script:
#!/bin/bash # Search for any remaining instances of the old variable names echo "Checking for any remaining instances of 'topOffset'..." rg "topOffset" echo "Verifying consistent usage of new 'offsetTop' and 'childOffsetTop'..." rg "offsetTop|childOffsetTop"Length of output: 9588
gimmyhehe
changed the title
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
Chores