Improve styling of buttons in toolbar & refactor CSS #8295
Conversation
…e primary color for selected buttons
…is not possible (like the other volume tools)
📝 WalkthroughWalkthroughThe pull request introduces enhancements to the frontend components, focusing on improving the user interface and interaction in the navbar and toolbar. Key modifications include the addition of a new Changes
Assessment against linked issues
Possibly related PRs
Suggested labels
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: 0
🧹 Nitpick comments (4)
frontend/javascripts/theme.tsx (1)
4-4: Consider grouping imports for clarity.Imports for
ConfigProvider,theme, andThemeConfigfrom "antd" have grown. A short re-grouping can increase readability by separating external vs. internal imports or by grouping from the same library together.frontend/javascripts/oxalis/view/layouting/tracing_layout_view.tsx (1)
49-49: Maintain consistent import references.Please confirm the relative or absolute import path matches your project structure. Sometimes referencing
themefrom the root vs. relative path can cause confusion if your directory structure changes.frontend/stylesheets/trace_view/_tracing_view.less (1)
411-411: Color reference check for hover state.
var(--color-blue-zircon)might not match your new or existing theme tokens. Ensure it complements yourColorWKBlueand other theme variables. This mismatch can cause jarring transitions between states.frontend/javascripts/oxalis/view/action-bar/toolbar_view.tsx (1)
422-433: ToggleButton usage for single-node tree mode.Neat approach to show an active/inactive state using
ToggleButton. Thestyleprop reference toNARROW_BUTTON_STYLEkeeps visual consistency.Consider adding a tooltip or context on hover that explains the benefits of single-node tree mode more thoroughly, if user feedback suggests confusion.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
frontend/javascripts/navbar.tsx(0 hunks)frontend/javascripts/oxalis/view/action-bar/toolbar_view.tsx(10 hunks)frontend/javascripts/oxalis/view/components/button_component.tsx(3 hunks)frontend/javascripts/oxalis/view/layouting/tracing_layout_view.tsx(3 hunks)frontend/javascripts/theme.tsx(2 hunks)frontend/stylesheets/trace_view/_tracing_view.less(2 hunks)
💤 Files with no reviewable changes (1)
- frontend/javascripts/navbar.tsx
🔇 Additional comments (17)
frontend/javascripts/theme.tsx (4)
18-39: Good job abstracting theme overrides into one object.Defining theme overrides in a single configuration object helps keep your styles consistent. This approach also makes it easier to share or merge these theme changes in other components. No pressing concerns here.
Line range hint
49-49: Confirm the usage ofNavAndStatusBarThemein consumer components.Exporting this theme is helpful, but ensure that all components using this theme are properly tested across various user themes, especially if toggling between dark and light modes.
90-90: Ensure colorPrimaryBg usage is correct.
colorPrimaryBgis set to the same value ascolorPrimary. Verify this is desired and won’t produce conflicts, especially in states where a distinct background color might be useful.
40-43: Verify merged themes for potential conflicts.Using
_.mergeis a good approach. However, confirm that merging these settings with the base dark theme does not override critical style tokens. For instance, ensure any custom tokens do not inadvertently conflict with the default tokens from your existing themes.✅ Verification successful
Let me search for the theme-related files and their contents to verify potential conflicts.
Theme merge implementation is safe and well-structured
The merge implementation is safe and well-designed. Here's why:
- The base theme (
getAntdTheme("dark")) and overrides (OverridesForNavbarAndStatusBarTheme) have clearly defined, non-conflicting token structures- The overrides only modify specific tokens with explicit values:
- Component-specific overrides for Radio and Button
- Core tokens like
colorPrimary,colorBgContainer,colorBorder, etc.- The base dark theme from Ant Design is preserved for all other tokens
- The custom overrides use consistent color variables (e.g.,
ColorWKBlue) that are reused across the theme🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Merging theme check: Grep for potential naming collisions or redefines of tokens rg -A 3 "token[:=]"Length of output: 86093
Script:
#!/bin/bash # Search for theme files and their contents fd theme -e tsx -e ts | xargs catLength of output: 4397
frontend/javascripts/oxalis/view/layouting/tracing_layout_view.tsx (2)
2-2: Imports look good.The inclusion of
ConfigProvideris consistent with the theming changes. No issues seen here.
302-354: Wrap withConfigProvideris a solid approach.Enclosing the content in
ConfigProviderto applyNavAndStatusBarThemeis a good practice. This promotes consistent styling across UI elements likeActionBarViewandAlert.frontend/stylesheets/trace_view/_tracing_view.less (1)
358-358: Ensure the variables match your theme tokens.
var(--ant-button-primary-color)andvar(--ant-color-primary)need to be validated against the new theme overrides for consistent styling. Double-check any unintentional misalignment between the CSS variables and the Ant tokens you have defined.frontend/javascripts/oxalis/view/components/button_component.tsx (3)
6-6: Good naming convention upgrade.Renaming from
ButtonComponentProptoButtonComponentPropsis clearer and aligns well with common naming practices for props interfaces/types.
15-16: Sufficient default property initialization.Using
_.noopas a default foronClickis a conscious design choice that avoids null checks. This is a neat way to ensure consistent prop usage.
64-67: Reusable toggle button approach.Exporting the
ToggleButtonfunction is a clean pattern to handle binary states. This is a good addition to keep code DRY when multiple components require a toggling UI.frontend/javascripts/oxalis/view/action-bar/toolbar_view.tsx (7)
55-55: Clarity of imports.Importing both
ButtonComponentandToggleButtonfrom the same module helps keep usage consistent.
170-170: Optional style prop.Typing
style?: React.CSSPropertiesis consistent with React best practices, ensuring that optional styling is typed and flexible.Also applies to: 216-216
Line range hint
434-449: Merger mode toggle logic.Using a
ToggleButtonfor merger mode is a natural choice. Theopacityanddisabledhandling make it clear when the button is inactive.
1079-1098: Quick Select tool integration.These lines correctly integrate
ToggleButtonfor the Quick Select tool, maintaining a consistent UI pattern.
Line range hint
1247-1259: Toggle AI-based Quick Select.The new
ToggleButtonto enable/disable AI-based heuristics is straightforward. The usage ofisAISelectAvailableensures graceful fallback.
1334-1342: Quick Select settings button.A dedicated toggling approach for the Popover is well-structured. The
ToggleButtonusage withfast_tooltippreserves a consistent UI feel.
1405-1424: Proofreading toggles.Adding separate toggles for mesh rendering and selective visibility keeps the user in control of the proofreading experience. This leads to a more modular approach.
… dark-theme-adapted primary color
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
frontend/javascripts/theme.tsx (2)
32-35: Add documentation for dark theme token.Consider adding a comment explaining the purpose and usage of
darkGlobalToken, as it's used in the navbar theme configuration.+// Pre-compute dark theme tokens for use in navbar and status bar theme overrides const darkGlobalToken = theme.getDesignToken({ token: globalDesignToken, algorithm: theme.darkAlgorithm, });
37-61: LGTM! Well-structured theme overrides with clear documentation.The navbar and status bar theme configuration is comprehensive and includes helpful comments. Consider using a more specific type for the merged theme.
-export const NavAndStatusBarTheme = _.merge( +export const NavAndStatusBarTheme: ThemeConfig = _.merge( getAntdTheme("dark"), OverridesForNavbarAndStatusBarTheme, );
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
frontend/javascripts/navbar.tsx(1 hunks)frontend/javascripts/theme.tsx(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- frontend/javascripts/navbar.tsx
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: circleci_build
🔇 Additional comments (3)
frontend/javascripts/theme.tsx (3)
4-4: LGTM! Well-organized imports and consistent color naming.The new imports and color constant additions are properly structured and follow the existing naming conventions.
Also applies to: 17-18
19-30: Verify primary color application for save button.The global design token correctly defines ColorWKBlue as the primary color. However, based on the PR description, there's an issue with the save button not showing this color.
Let's verify how antd applies these tokens to buttons:
106-112: Verify the theme mutation fix.The clone solution prevents theme mutation, but let's verify if this is the best approach or if there's a recommended way from antd.
Let's search for similar issues or patterns:
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
frontend/javascripts/oxalis/view/layouting/tracing_layout_view.tsx (1)
303-308: Consider extracting inline stylesThe inline styles could be moved to a separate constant or CSS module for better maintainability. Additionally, the zIndex value (210) should be defined in a constants file to maintain consistency across the application.
+ // In a constants file + export const Z_INDICES = { + TOOLBAR: 210, + } as const; + + // In a styles file + export const toolbarStyles = { + flex: "0 1 auto", + zIndex: Z_INDICES.TOOLBAR, + display: "flex", + } as const; - style={{ - flex: "0 1 auto", - zIndex: 210, - display: "flex", - }} + style={toolbarStyles}frontend/javascripts/oxalis/view/action-bar/toolbar_view.tsx (1)
422-433: Consider extracting common ToggleButton props to reduce duplicationThe ToggleButton implementations are consistent and well-structured. However, there's an opportunity to improve code maintainability by extracting common props into a reusable interface or helper.
Consider this approach:
interface CommonToggleButtonProps { style?: React.CSSProperties; tooltipPlacement?: TooltipPlacement; className?: string; } const defaultToggleButtonProps: CommonToggleButtonProps = { style: NARROW_BUTTON_STYLE, tooltipPlacement: 'right', className: 'narrow' }; // Usage <ToggleButton {...defaultToggleButtonProps} active={isActive} title={tooltipText} onClick={handleClick} > {children} </ToggleButton>Also applies to: 434-449, 1247-1259, 1334-1342, 1440-1447, 1448-1459
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
CHANGELOG.unreleased.md(1 hunks)frontend/javascripts/oxalis/view/action-bar/toolbar_view.tsx(10 hunks)frontend/javascripts/oxalis/view/layouting/tracing_layout_view.tsx(3 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- CHANGELOG.unreleased.md
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: circleci_build
🔇 Additional comments (4)
frontend/javascripts/oxalis/view/layouting/tracing_layout_view.tsx (2)
48-48: LGTM: Theme import aligns with styling improvementsThe addition of
NavAndStatusBarThemeimport supports the PR's objective of improving toolbar styling.
300-327: LGTM: Clean theme integration for toolbarThe integration of
ConfigProviderwithNavAndStatusBarThemeeffectively implements the toolbar styling improvements mentioned in the PR objectives.frontend/javascripts/oxalis/view/action-bar/toolbar_view.tsx (2)
55-55: LGTM: Clean import addition for ToggleButtonThe import statement is well-structured and follows TypeScript conventions.
1079-1098: LGTM: Well-implemented Quick Select toolThe Quick Select tool implementation is well-structured and consistent with other tools in the toolbar. It properly handles:
- Accessibility through proper alt text
- Disabled states with opacity changes
- Consistent styling with other tools
|
Nitpick: The button of the proof reading tool is even narrow then before: 
|
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
frontend/javascripts/oxalis/view/action-bar/toolbar_view.tsx (1)
Line range hint
1-1500: Excellent architectural improvements to the toolbar!The changes demonstrate good architectural decisions:
- Consistent use of reusable components
- Clear separation of concerns
- Proper state management
- Good balance between functionality and user experience
Consider extracting some of the larger components (like QuickSelect related components) into separate files to improve maintainability if this file grows further.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
frontend/javascripts/components/fast_tooltip.tsx(1 hunks)frontend/javascripts/oxalis/view/action-bar/toolbar_view.tsx(15 hunks)
✅ Files skipped from review due to trivial changes (1)
- frontend/javascripts/components/fast_tooltip.tsx
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: circleci_build
🔇 Additional comments (7)
frontend/javascripts/oxalis/view/action-bar/toolbar_view.tsx (7)
55-55: Great improvement using the ToggleButton component!The introduction and usage of the
ToggleButtoncomponent across various UI elements provides better visual feedback for active states and improves code consistency. The implementation follows React best practices by reusing components effectively.Also applies to: 427-438, 439-454, 1244-1256, 1331-1339, 1437-1444, 1445-1456
1084-1103: Well-implemented QuickSelect tool with AI integration!The implementation includes:
- Clear user feedback through tooltips
- Proper feature flag handling for AI capabilities
- Graceful degradation when AI is not available
- Accessible UI elements with appropriate icons and labels
Also applies to: 1213-1256
1437-1456: Excellent UX improvements in ProofReadingComponents!The refactoring to use
ToggleButtoncomponents with clear tooltips and consistent styling enhances user understanding and interaction with the proofreading tools.
170-170: Good addition of flexible styling capabilities!Making the style props optional improves component reusability while maintaining type safety with TypeScript.
Also applies to: 216-216
1144-1144: Verify if the padding adjustment resolves the button width issue.The padding addition of
0 4pxwas made to address the reported narrower width of the proofreading tool button.Please confirm if this adjustment matches the width of other toolbar buttons and provides consistent spacing.
Line range hint
1284-1322: Well-implemented user onboarding for new AI features!The NuxPopConfirm component effectively introduces new AI capabilities to users with:
- Clear and informative messaging
- Proper state management for user experience
- Good use of Ant Design components
Line range hint
1323-1342: Clean implementation of QuickSelectSettingsPopover!The component effectively combines:
- Proper integration with user onboarding
- Consistent use of UI patterns
- Clean state management
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (4)
frontend/javascripts/theme.tsx (4)
17-17: Consider adding a descriptive comment for the new color constant.The color constant follows the naming convention, but adding a comment describing its purpose (like the other color constants) would improve maintainability.
+// Dark background color for navbar and status bar const ColorDarkBg = "#383d48";
20-30: Consider using a more specific type for the design token.The configuration is well-structured and uses the brand colors consistently. However, using
Partial<AliasToken>might be too permissive.Consider creating a type that explicitly lists the token properties you're customizing:
type WKDesignToken = Pick<AliasToken, | 'colorPrimary' | 'colorLink' | 'colorLinkHover' | 'colorInfo' | 'blue' | 'borderRadius' | 'fontFamily' | 'colorPrimaryBg' >;
37-60: Consider using component-specific types for better type safety.The theme configuration is well-structured and addresses the button styling requirements. The comment explaining the non-transparent disabled background is particularly helpful.
Consider using component-specific types for better type safety:
type NavbarThemeComponents = { Radio: { buttonCheckedBg: string; buttonSolidCheckedBg: string; buttonBg: string; }; Button: { primaryShadow: string; }; }; const OverridesForNavbarAndStatusBarTheme: ThemeConfig & { components: NavbarThemeComponents; } = { // ... existing configuration };
105-111: Consider adding type assertion for better type safety.The clone operation effectively prevents theme mutation. However, the return type could be more explicit.
Consider adding type assertion for better type safety:
type AntdThemeConfig = { algorithm: typeof theme.defaultAlgorithm | typeof theme.darkAlgorithm; token: Partial<AliasToken>; components: OverrideToken; }; return { algorithm, token: _.clone(globalDesignToken), components, } as AntdThemeConfig;
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
frontend/javascripts/theme.tsx(2 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: circleci_build
🔇 Additional comments (2)
frontend/javascripts/theme.tsx (2)
4-4: LGTM: Import statements are properly structured.The new imports from
antdare necessary for the theme customization functionality.
32-35: LGTM: Dark theme token generation is properly implemented.The code correctly uses antd's dark algorithm to generate the dark theme token.
URL of deployed dev instance (used for testing):
Steps to test:
TODOs:
Issues:
(Please delete unneeded items, merge only when none are left open)