-
Notifications
You must be signed in to change notification settings - Fork 322
fix(nav-menu): [nav-menu] add dark theme #3097
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
Conversation
WalkthroughThis pull request updates the navigational styling in the theme. In the LESS files, a box shadow on the main navigation menu has been replaced with a solid border, the hover effect on the Changes
Possibly related PRs
Suggested labels
Suggested reviewers
Poem
🪧 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 (
|
WalkthroughThis PR introduces a dark theme to the Changes
|
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
🧹 Nitpick comments (2)
packages/theme/src/nav-menu/vars.less (2)
34-35
: Dark Theme: Popmenu Background Variable AddedThe new variable
--tv-NavMenu-popmenu-box-background: var(--tv-color-bg-hover-4);
has been introduced to define the dropdown menu’s background in the dark theme. Please verify that--tv-color-bg-hover-4
is defined in the theme’s color palette and provides adequate contrast and visual consistency in dark mode.
45-45
: Style Update: Popmenu More Menu Border Color ChangeThe update of
--tv-NavMenu-popmenu-more-menu-border-color-right
tovar(--tv-color-border-disabled)
should help the dropdown border better match the overall dark theme aesthetics. Please ensure that this modification does not compromise the visual clarity or accessibility of border elements in various display contexts.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
packages/theme/src/nav-menu/index.less
(2 hunks)packages/theme/src/nav-menu/vars.less
(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- packages/theme/src/nav-menu/index.less
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: PR E2E Test (pnpm test:e2e3)
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: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
packages/theme/src/nav-menu/index.less
(1 hunks)packages/theme/src/nav-menu/vars.less
(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- packages/theme/src/nav-menu/index.less
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: PR E2E Test (pnpm test:e2e3)
🔇 Additional comments (1)
packages/theme/src/nav-menu/vars.less (1)
43-43
: Update More Menu Border Color for Disabled StateLine 43 updates the more menu's right border color to
var(--tv-color-border-disabled)
. This change is consistent with a design approach aimed at a more subdued (disabled) look, which can be appropriate for a dark theme. Ensure that this change integrates well with other dark theme elements and does not inadvertently affect any interactive feedback or accessibility considerations.
// 导航菜单栏项选中下划线背景色 | ||
--tv-NavMenu-item-selected-underline-bg-color: var(--tv-color-bg-primary); | ||
// 导航菜单菜单栏下拉框阴影 | ||
--tv-NavMenu-popmenu-box-shadow: var(--tv-shadow-3-down); | ||
// 导航菜单菜单栏下拉框选中文本色 | ||
--tv-NavMenu-popmenu-selected-text-color: var(--tv-color-text); | ||
// 导航菜单菜单栏下拉背景色 | ||
--tv-NavMenu-popmenu-bg-color: var(--tv-color-bg-3, #fff); | ||
--tv-NavMenu-popmenu-bg-color: var(--tv-color-bg-hover-4, #fff); |
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.
@@ -23,7 +23,7 @@ | |||
height: var(--tv-NavMenu-height); | |||
position: relative; | |||
padding: 0 var(--tv-NavMenu-padding); | |||
box-shadow: 0 1px 3px 1px rgba(0, 0, 0, 0.1); | |||
border: 1px solid var(--tv-color-border-disabled); |
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.
1 首先这里要用组件级变量
2 场景不对,这里不是禁用场景的话不该用禁用边框色,而是常规边框色
3 关于阴影,设计规范有暗黑主题的阴影,具体在tokens的表格里
// 导航菜单菜单栏下拉框选中字重 | ||
--tv-NavMenu-popmenu-selected-font-weight: var(--tv-font-weight-bold); | ||
// 导航菜单菜单栏下拉框文本色 | ||
--tv-NavMenu-popmenu-text-color: var(--tv-color-text); | ||
// 导航菜单更多按钮展示的下拉框右边框色 | ||
--tv-NavMenu-popmenu-more-menu-border-color-right: var(--tv-color-border); | ||
--tv-NavMenu-popmenu-more-menu-border-color-right: var(--tv-color-border-disabled); |
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.
场景不对,如果确实是缺场景,要补充对应场景的变量
重复 |
repeat |
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