-
Notifications
You must be signed in to change notification settings - Fork 273
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
feat(anchor): [anchor] Added additional anchor features and modified document issues such as icons found in the document. #2313
Conversation
WalkthroughThe pull request introduces modifications to 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 (
|
d9004b0
to
7c7b659
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (5)
packages/theme/src/anchor/index.less (5)
63-83
: Enhanced anchor link styling and structureThe new nested styles for the anchor link structure significantly improve the layout and visual hierarchy. The use of CSS variables for margins and colors is excellent for maintainability and theming. The specific padding, font size, and line height values enhance readability and consistency.
One minor suggestion for improvement:
Consider using a CSS variable for the font size (12px) to maintain consistency with other parts of the component:
&-link-title { padding: 7px 8px 7px 16px; - font-size: 12px; + font-size: var(--tv-Anchor-link-font-size, 12px); line-height: 1.5; }This change would allow for easier global updates to the font size if needed in the future.
108-108
: Enhanced theming for active orbit skidThe update to use
var(--tv-Anchor-link-skid-bg-color-active)
for the active state background color of the orbit skid is a good improvement. This change allows for more granular control over the active state appearance and enhances the theming capabilities of the component.To further improve consistency and maintainability:
Consider adding a transition property to smooth the color change when the skid becomes active:
&-skid { position: absolute; left: 0; width: var(--tv-Anchor-orbit-skid-width); background-color: var(--tv-Anchor-orbit-bg-color); + transition: background-color 0.3s ease; &--active { background-color: var(--tv-Anchor-link-skid-bg-color-active); } }
This addition would create a more polished user experience by smoothly transitioning between the default and active states.
125-125
: Improved spacing for dot-style anchor linksThe addition of an 8px left margin to the anchor link within the dot style improves the spacing and alignment of the links. This change enhances the visual hierarchy and readability of the component.
To improve flexibility and maintain consistency with the component's theming approach:
Consider using a CSS variable for the margin value:
.@{anchor-prefix-cls}-link { position: relative; padding-left: 10px; margin-bottom: 16px; - margin-left: 8px; + margin-left: var(--tv-Anchor-dot-link-margin-left, 8px); }This change would allow for easier customization of the spacing while providing a default value of 8px.
129-129
: Refined padding for dot-style link titlesThe update to the padding (1px 0 1px 16px) for the anchor link title in the dot style provides more precise control over the spacing. This change likely improves the alignment of the link title with the dot indicator, enhancing the overall visual appearance of the component.
To maintain consistency with the component's theming approach and improve flexibility:
Consider using CSS variables for the padding values:
.@{anchor-prefix-cls}-link-title { - padding: 1px 0 1px 16px; + padding: var(--tv-Anchor-dot-link-title-padding-top, 1px) 0 + var(--tv-Anchor-dot-link-title-padding-bottom, 1px) + var(--tv-Anchor-dot-link-title-padding-left, 16px); -webkit-line-clamp: 2; }This change would allow for easier customization of the padding while providing default values that match the current design.
63-84
: Comprehensive improvement to anchor link stylingThe nested styles for the anchor link structure significantly enhance the layout and visual hierarchy of the component. The use of CSS variables for margins and colors is excellent for maintainability and theming. The specific padding, font size, and line height values improve readability and consistency.
To further improve consistency and maintainability:
- Consider using CSS variables for all specific values, including paddings and line height:
&-link-title { - padding: 7px 8px 7px 16px; + padding: var(--tv-Anchor-link-title-padding, 7px 8px 7px 16px); font-size: 12px; - line-height: 1.5; + line-height: var(--tv-Anchor-link-title-line-height, 1.5); &--active { background-color: var(--tv-Anchor-link-bg-color-active); } }
- Add a transition for smooth color changes when hovering or activating links:
&-link-title { /* ... existing styles ... */ + transition: color 0.3s ease, background-color 0.3s ease; }
These changes would further enhance the component's flexibility and user experience.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (9)
- examples/sites/demos/apis/anchor.js (1 hunks)
- examples/sites/demos/pc/app/icon/show-title-composition-api.vue (0 hunks)
- examples/sites/demos/pc/app/icon/show-title.spec.ts (0 hunks)
- examples/sites/demos/pc/app/icon/show-title.vue (0 hunks)
- examples/sites/demos/pc/app/icon/webdoc/icon.js (0 hunks)
- packages/theme/src/anchor/index.less (4 hunks)
- packages/theme/src/anchor/vars.less (1 hunks)
- packages/theme/src/tabs/index.less (0 hunks)
- packages/theme/src/tabs/vars.less (1 hunks)
💤 Files with no reviewable changes (5)
- examples/sites/demos/pc/app/icon/show-title-composition-api.vue
- examples/sites/demos/pc/app/icon/show-title.spec.ts
- examples/sites/demos/pc/app/icon/show-title.vue
- examples/sites/demos/pc/app/icon/webdoc/icon.js
- packages/theme/src/tabs/index.less
🧰 Additional context used
🔇 Additional comments (9)
packages/theme/src/anchor/vars.less (5)
25-25
: LGTM. Verify visual impact of reduced margin.The left margin for anchor links has been reduced from 8px to 4px. This change aligns with the PR objective of modifying anchor features.
Please verify that this reduction in margin doesn't negatively impact the visual layout or readability of the anchor links in various scenarios.
28-29
: LGTM. New variable for active link background color.A new CSS variable has been added to define the background color for active anchor links. This addition enhances the customization options for the component.
Please ensure that this new background color for active links provides sufficient contrast with the text color and aligns with the overall design system.
30-31
: LGTM. New variable for active skid background color.A new CSS variable has been added to define the background color for the active skid of anchor links. This addition provides more granular control over the component's appearance.
Please confirm that this new background color for the active skid provides a visually distinct and accessible indicator of the active state.
37-37
: LGTM. Verify impact of transparent link mask.The background color for the anchor link mask has been changed from a semi-transparent blue to transparent. This modification aligns with the PR objective of updating the component's appearance.
Please ensure that removing the semi-transparent background doesn't negatively impact the user experience or visual feedback when interacting with anchor links. Consider testing this change across different themes and color schemes to confirm its effectiveness.
Line range hint
1-53
: Verify impact of removed variables.Two variables have been removed from this file:
--tv-Anchor-link-bg-color-active
--tv-Anchor-link-title-padding-right
While the first variable seems to be replaced by a new one with a slightly different value, the second variable's removal might affect the layout of anchor link titles.
Please confirm that:
- The replacement of
--tv-Anchor-link-bg-color-active
with the new variable doesn't introduce any unintended visual changes.- The removal of right padding for anchor link titles (
--tv-Anchor-link-title-padding-right
) doesn't negatively impact the layout or readability of the component.Consider updating any components or styles that might have been using these removed variables to ensure consistent behavior across the application.
examples/sites/demos/apis/anchor.js (1)
87-87
: Improved type signature formatting.The update to the
link-click
event type signature enhances readability and maintains consistency with common TypeScript formatting practices.packages/theme/src/anchor/index.less (2)
Line range hint
51-55
: Improved active state styling with increased specificityThe addition of the
&&--active
selector for the link title is a good improvement. The use of double ampersand (&&) increases the specificity of the selector, which ensures that the active state styles will take precedence over the default styles. This is a good practice for maintaining style consistency and avoiding potential conflicts.
88-88
: Improved flexibility with calc() and CSS variableThe update to use
calc(var(--tv-Anchor-orbit-width) / 2)
for the left position of the anchor link mask is a good improvement. This change allows for more flexible and maintainable positioning, as it automatically adjusts based on the orbit width. It ensures better alignment between the mask and the orbit element, improving the overall visual consistency of the component.packages/theme/src/tabs/vars.less (1)
99-99
: Approve the dark mode background color change with a suggestion for verification.The change from
var(--tv-color-bg-secondary, #ffffff)
tovar(--tv-color-bg-primary, #191919)
for the--tv-Tabs-header-dark-bg-color
variable is a good improvement for dark mode aesthetics. It now properly uses a dark background color instead of a light one.Please verify that the contrast between this new background color and the text/icons in the tab header is sufficient for readability in dark mode. You may want to run an accessibility check to ensure the color contrast meets WCAG standards.
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
on-change
event from the anchor component API.Style
Chores