[WEB-4082] 14.9.0 - Adopt TW theming, real SB dark mode, add TS declarations for scripts #546
Conversation
|
Important Review skippedAuto incremental reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the You can disable this status message by setting the WalkthroughThe changes in this pull request introduce a dark theme variant for the Storybook application by adding new CSS rules and modifying existing ones in 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 (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
🧹 Outside diff range and nitpick comments (20)
.storybook/application.css (1)
46-51: Consider consolidating selectors for better maintainabilityThe multiple selectors targeting text color could be simplified.
Consider consolidating these selectors:
-.ui-theme-dark .sbdocs-content > *, -.ui-theme-dark .sb-anchor > h3, -.ui-theme-dark .sb-anchor > p, -.ui-theme-dark .sb-anchor > p > code { +.ui-theme-dark .sbdocs-content > *, +.ui-theme-dark .sb-anchor :where(h3, p, p > code) { @apply text-neutral-000; }tsc.js (1)
67-67: Consider making additional modules configurableWhile the implementation works, hardcoding the module names in the script makes it less flexible. Consider moving this to a configuration file or accepting it as a command-line argument.
Example implementation using command-line arguments:
-const additionalModules = ["@ably/ui/core/scripts"]; +const additionalModules = process.argv.slice(2).length > 0 + ? process.argv.slice(2) + : ["@ably/ui/core/scripts"];Or using a configuration file:
+const config = require('./tsc.config.json'); -const additionalModules = ["@ably/ui/core/scripts"]; +const additionalModules = config.additionalModules || [];src/core/ProductTile.tsx (4)
26-34: Well-structured dark mode implementationThe dark mode implementation follows good practices with:
- Consistent use of neutral color scale
- Proper transition handling
- Appropriate hover states
Consider extracting these common class combinations into CSS classes to reduce the inline complexity:
.product-tile { @apply rounded-lg p-12 flex flex-col gap-8 transition-colors; } .product-tile--selected { @apply bg-neutral-1200 dark:bg-neutral-100; } .product-tile--default { @apply bg-neutral-100 dark:bg-neutral-1200 hover:bg-neutral-200 dark:hover:bg-neutral-1100; }
39-51: Consider simplifying icon styling logicWhile the dark mode implementation is correct, the conditional props could be simplified to reduce cognitive load.
Consider consolidating the className logic:
-className={ - selected - ? "from-neutral-900 dark:from-neutral-400" - : "from-neutral-400 dark:from-neutral-900" -} -innerClassName={ - selected - ? "bg-neutral-1100 dark:bg-neutral-200" - : "bg-neutral-200 dark:bg-neutral-1100" -} +className={selected ? "from-neutral-900 dark:from-neutral-400" : "from-neutral-400 dark:from-neutral-900"} +innerClassName={selected ? "bg-neutral-1100 dark:bg-neutral-200" : "bg-neutral-200 dark:bg-neutral-1100"}
61-77: Consider extracting repeated dark mode patternsWhile the text styling implementation is thorough, there's repetition in the dark/light mode color patterns that could be simplified.
Consider creating utility classes for these common patterns:
.text-adaptive-primary { @apply text-neutral-1000 dark:text-neutral-300; &.selected { @apply text-neutral-300 dark:text-neutral-1000; } } .text-adaptive-secondary { @apply text-neutral-800 dark:text-neutral-500; &.selected { @apply text-neutral-000 dark:text-neutral-1300; } }This would simplify the inline conditions and make the patterns more maintainable.
Also applies to: 91-95
101-101: Move inline styles to a CSS classThe long inline
additionalCSSstring makes the code harder to maintain and modify.Consider moving these styles to a CSS class:
.featured-link-selected { @apply ui-btn-secondary bg-transparent hover:bg-transparent w-full hover:text-neutral-1300 mt-8 text-center inline-block text-neutral-000 dark:text-neutral-1300; }Then simplify the component usage:
-additionalCSS="ui-btn-secondary bg-transparent hover:bg-transparent w-full hover:text-neutral-1300 mt-8 text-center inline-block text-neutral-000 dark:text-neutral-1300" +additionalCSS="featured-link-selected"src/core/Icon/Icon.stories.tsx (2)
69-69: Good architectural improvement moving to CSS-based theming!The consolidation of light/dark variants into a single story using CSS-based theming is a cleaner approach that better aligns with Tailwind's design principles.
80-111: Consider refactoring for better maintainabilityWhile the implementation correctly handles dark mode, consider these improvements:
- Extract repeated Icon components into a reusable function
- Create a shared container component for consistent spacing
Here's a suggested refactor:
+ const IconExample = ({ color, secondaryColor }: { color: string; secondaryColor?: string }) => ( + <Icon + name="icon-gui-check-circled-fill" + color={color} + secondaryColor={secondaryColor} + size="1.5rem" + /> + ); + + const Section = ({ title, children }: { title: string; children: React.ReactNode }) => ( + <div className="flex gap-24"> + <h4 className="ui-text-h4 text-neutral-1300 dark:text-neutral-000"> + {title}: + </h4> + {children} + </div> + ); export const IconWithSecondaryColor = { render: () => ( <div className="flex items-center justify-center p-16 gap-24"> - <div className="flex gap-24"> - <h4 className="ui-text-h4 text-neutral-1300 dark:text-neutral-000"> - Non-themed: - </h4> - <Icon - name="icon-gui-check-circled-fill" - color="text-orange-600" - size="1.5rem" - /> - <Icon - name="icon-gui-check-circled-fill" - color="text-orange-600" - secondaryColor="text-neutral-1300" - size="1.5rem" - /> - </div> + <Section title="Non-themed"> + <IconExample color="text-orange-600" /> + <IconExample + color="text-orange-600" + secondaryColor="text-neutral-1300" + /> + </Section> - <div className="flex gap-24"> - <h4 className="ui-text-h4 text-neutral-1300 dark:text-neutral-000"> - Themed: - </h4> - <Icon - name="icon-gui-check-circled-fill" - color="text-orange-600" - size="1.5rem" - /> - <Icon - name="icon-gui-check-circled-fill" - color="text-orange-200 dark:text-orange-1000" - secondaryColor="text-orange-600 dark:text-orange-600" - size="1.5rem" - /> - </div> + <Section title="Themed"> + <IconExample color="text-orange-600" /> + <IconExample + color="text-orange-200 dark:text-orange-1000" + secondaryColor="text-orange-600 dark:text-orange-600" + /> + </Section> </div> ),src/core/Tooltip.tsx (1)
182-184: Consider improving template literal readabilityWhile the implementation correctly handles dark mode and animations, the template literal could be more readable.
Consider breaking down the className into multiple lines:
- className={`bg-neutral-1000 dark:bg-neutral-300 text-neutral-200 dark:text-neutral-1000 ui-text-p3 font-medium p-16 ${interactive ? "" : "pointer-events-none"} rounded-lg absolute ${tooltipProps?.className ?? ""} ${fadeOut ? "animate-[tooltipExit_0.25s_ease-in-out]" : "animate-[tooltipEntry_0.25s_ease-in-out]"}`} + className={` + bg-neutral-1000 dark:bg-neutral-300 + text-neutral-200 dark:text-neutral-1000 + ui-text-p3 font-medium p-16 + ${interactive ? "" : "pointer-events-none"} + rounded-lg absolute + ${tooltipProps?.className ?? ""} + ${fadeOut ? "animate-[tooltipExit_0.25s_ease-in-out]" : "animate-[tooltipEntry_0.25s_ease-in-out]"} + `}src/core/Pricing/data.tsx (3)
100-100: Consider visual differentiation between Standard and Pro plansThe Pro plan currently uses identical colors to the Standard plan:
// Both plans use: text-neutral-1300 dark:text-neutral-000 // for title text-neutral-800 dark:text-neutral-500 // for description text-blue-600/400 dark:text-blue-400/800 // for list itemsThis might make it difficult for users to visually distinguish between the two tiers. Consider using a different accent color or intensity for the Pro plan to create a clear visual hierarchy.
Also applies to: 105-105, 137-138
201-201: Consider extracting repeated color schemes into constantsThe consumption data sections (Messages, Channels, Connections) use identical color schemes:
title: text-neutral-1300 dark:text-neutral-000 description: text-neutral-700 dark:text-neutral-600Consider extracting these into named constants to improve maintainability:
const CONSUMPTION_COLORS = { title: "text-neutral-1300 dark:text-neutral-000", description: "text-neutral-700 dark:text-neutral-600", } as const; // Usage { title: { color: CONSUMPTION_COLORS.title, // ... }, description: { color: CONSUMPTION_COLORS.description, // ... }, }Also applies to: 207-207, 229-229, 242-242, 264-264, 277-277
Dark mode implementation needs to be updated
The dark mode classes in the code use the
dark:prefix, but the project's Tailwind configuration uses a custom selector.ui-theme-dark. This mismatch means the dark mode styles won't work as intended.
- Update all
dark:prefixes insrc/core/Pricing/data.tsxto useui-theme-dark:instead- Examples of affected classes:
dark:text-neutral-700→ui-theme-dark:text-neutral-700dark:text-neutral-000→ui-theme-dark:text-neutral-000dark:text-blue-400→ui-theme-dark:text-blue-400- And similar patterns throughout the file
🔗 Analysis chain
Line range hint
1-294: Verify dark mode configurationLet's ensure the dark mode class prefix used in this file (
dark:) aligns with the project's Tailwind configuration.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Check Tailwind config for dark mode settings rg -g 'tailwind.config.{js,ts}' 'darkMode' # Check for any other dark mode related configurations rg -g '*.{js,ts,tsx,css}' 'ui-theme-dark|dark-mode|darkMode'Length of output: 231
src/core/Pricing/PricingCards.tsx (4)
Line range hint
66-89: Consider improving type safety with literal typesThe color mapping implementation looks good with proper dark mode support. However, we can improve type safety further.
Consider using literal types for the classes record:
- const classes: Record< - string, - { - border: string; - bg: string; - } - > = { + const classes: Record< + 'neutral' | 'blue' | 'orange', + { + border: string; + bg: string; + } + > = {
110-116: Consider extracting common classes for better maintainabilityThe conditional classes are well-structured, but extracting common classes could improve readability and maintainability.
Consider refactoring like this:
+ const baseCardClasses = "relative border flex-1 px-24 py-32 flex flex-col gap-24 rounded-2xl group min-w-[272px] backdrop-blur"; + const responsiveClasses = { "@[520px]:flex-row @[920px]:flex-col": delimiter }; + const borderClasses = borderClasses(border?.color)?.border ?? "border-neutral-200 dark:border-neutral-1100"; className={clsx( - "relative border flex-1 px-24 py-32 flex flex-col gap-24 rounded-2xl group min-w-[272px] backdrop-blur", - borderClasses(border?.color)?.border ?? "border-neutral-200 dark:border-neutral-1100", - border?.style, - { "@[520px]:flex-row @[920px]:flex-col": delimiter }, + baseCardClasses, + borderClasses, + border?.style, + responsiveClasses, )}
188-192: Extract default CTA classes for better readabilityThe CTA button styling is well-implemented with proper dark mode support, but the default class string could be extracted for better maintainability.
Consider this improvement:
+ const defaultCtaClasses = "ui-btn bg-neutral-1300 dark:bg-neutral-000 text-neutral-000 dark:text-neutral-1300"; additionalCSS={clsx( "text-center px-24 !py-12 hover:text-neutral-000 cursor-pointer", - cta.className ?? "ui-btn bg-neutral-1300 dark:bg-neutral-000 text-neutral-000 dark:text-neutral-1300", + cta.className ?? defaultCtaClasses, )}
Color contrast ratios do not meet WCAG requirements in dark mode
Based on the CSS color values found, the following combinations fail WCAG 2.1 Level AA requirements:
text-neutral-500(#c6ced9) onbg-neutral-1300(#03020d) - contrast ratio below 4.5:1text-neutral-300(#e6eaf0) onbg-blue-900(#001b2a) - contrast ratio too high, causing visual straintext-neutral-800(#667085) on dark backgrounds - contrast ratio below 4.5:1Recommendations:
- For secondary text in dark mode, use
text-neutral-400instead oftext-neutral-500- For table rows, use
text-neutral-200instead oftext-neutral-300- For hover states, use
text-neutral-600instead oftext-neutral-800🔗 Analysis chain
Line range hint
1-289: Verify color contrast ratios for dark modeWhile the dark mode implementation looks good, it's important to verify that all color combinations meet WCAG contrast requirements, especially for text and interactive elements.
Color contrast ratios need attention in dark mode
Several color combinations require verification for WCAG compliance:
- Text colors with potential contrast issues:
text-neutral-500on various backgrounds in dark modetext-neutral-300on potential blue background (bg-blue-900)- Interactive elements using
text-neutral-800→text-neutral-500color transition on hoverSince these color values are likely defined in a theme configuration file, let's check for it:
Color contrast issues identified in dark mode
Several color combinations in dark mode require attention for WCAG compliance:
text-neutral-500onbg-neutral-1300(used for secondary text)text-neutral-300onbg-blue-900(used in pricing table rows)text-neutral-800on dark backgrounds during hover statesLet's verify if there are any CSS custom properties or theme configurations that define these specific color values:
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for all color classes used in text elements rg -A 1 'text-[a-zA-Z]+-[0-9]+' src/core/Pricing/PricingCards.tsx # Search for all background color classes rg -A 1 'bg-[a-zA-Z]+-[0-9]+' src/core/Pricing/PricingCards.tsxLength of output: 3068
Script:
#!/bin/bash # Search for theme/color configuration files fd -e js -e ts -e json "tailwind|theme|color" --type f # Look for color value definitions rg -A 2 "neutral-[0-9]+" --type ts --type js --type jsonLength of output: 26460
Script:
#!/bin/bash # Search for CSS files with color definitions fd -e css . --type f --exec rg -A 1 '(--color-neutral-[0-9]+|--color-blue-900)' # Search for any theme configuration files fd "theme|tailwind" --type f --exec rg -A 1 '(neutral-[0-9]+|blue-900)'Length of output: 2892
🧰 Tools
🪛 Biome
[error] 167-167: The assignment should not be in an expression.
The use of assignments in expressions is confusing.
Expressions are often considered as side-effect free.(lint/suspicious/noAssignInExpressions)
tailwind.config.js (2)
Line range hint
1-400: Well-structured theming architecture using CSS variablesThe configuration demonstrates a robust theming approach:
- Extensive use of CSS variables enables dynamic theme switching
- Comprehensive color system with semantic naming
- GUI-specific colors are properly isolated
- No hardcoded color values
Consider documenting the CSS variable structure in a separate theme documentation file to help other developers understand the theming system.
Line range hint
401-450: Consider documenting animation timing and usageThe animation configurations for tooltips and accordions are well-implemented, but could benefit from documentation about their intended usage and accessibility considerations.
Consider adding comments above the animation configurations to document:
- Usage guidelines
- Timing considerations
- Accessibility implications (respecting reduced motion preferences)
keyframes: { + // Entry animation for tooltips - 300ms duration with opacity and transform + // Note: Should respect prefers-reduced-motion tooltipEntry: { "0%": { transform: "translateY(8px)", opacity: 0 }, "100%": { transform: "translateY(0)", opacity: 1 }, },src/core/Icon.tsx (2)
73-82: OptimizeuseCallbackdependencies to prevent unnecessary re-rendersIncluding
additionalAttributesin the dependency array ofuseCallbackcan causeiconSvgto be recreated on every render ifadditionalAttributesis not memoized, potentially impacting performance.Consider the following options:
Option 1: If
additionalAttributesare unlikely to change frequently, you can exclude it from the dependency array.Option 2: Memoize
additionalAttributesusinguseMemoif the attributes depend on dynamic values.Example of Option 1:
const iconSvg = useCallback( (secondaryColor?: ColorClass, isThemed?: boolean) => { // existing code }, [ name, size, color, additionalCSS, // Removed `additionalAttributes` from dependencies lightSecondaryColor, darkSecondaryColor, ], );
84-93: Consider consolidating SVG rendering to a single elementRendering multiple
<svg>elements for light and dark themes may affect performance and increase DOM complexity, especially when rendering many icons.Explore using CSS variables and media queries to handle theming within a single SVG element. This can improve performance and simplify the component.
Example approach:
Define CSS variables for colors:
:root { --icon-secondary-color: /* default color */; } @media (prefers-color-scheme: dark) { :root { --icon-secondary-color: /* dark theme color */; } }Update the SVG to use the CSS variable:
<svg className={clsx( { [`${color}`]: color }, { [`${additionalCSS}`]: additionalCSS }, - { "hidden dark:block": /* condition */ }, - { "dark:hidden": /* condition */ }, )} style={ { width: size, height: size, "--ui-icon-secondary-color": secondaryColorValue || "var(--icon-secondary-color)", } as CSSProperties } {...additionalAttributes} >This approach reduces the number of DOM elements and leverages CSS for theming.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
⛔ Files ignored due to path filters (12)
src/core/ContactFooter/__snapshots__/ContactFooter.stories.tsx.snapis excluded by!**/*.snapsrc/core/Expander/__snapshots__/Expander.stories.tsx.snapis excluded by!**/*.snapsrc/core/FeaturedLink/__snapshots__/FeaturedLink.stories.tsx.snapis excluded by!**/*.snapsrc/core/Footer/__snapshots__/Footer.stories.tsx.snapis excluded by!**/*.snapsrc/core/Icon/__snapshots__/Icon.stories.tsx.snapis excluded by!**/*.snapsrc/core/ProductTile/__snapshots__/ProductTile.stories.tsx.snapis excluded by!**/*.snapsrc/core/Slider/__snapshots__/Slider.stories.tsx.snapis excluded by!**/*.snapsrc/core/Table/__snapshots__/Table.stories.tsx.snapis excluded by!**/*.snapsrc/core/Tooltip/__snapshots__/Tooltip.stories.tsx.snapis excluded by!**/*.snapsrc/core/styles/__snapshots__/Button.stories.tsx.snapis excluded by!**/*.snapsrc/core/styles/__snapshots__/Forms.stories.tsx.snapis excluded by!**/*.snapsrc/core/styles/colors/__snapshots__/Colors.stories.tsx.snapis excluded by!**/*.snap
📒 Files selected for processing (21)
.storybook/application.css(1 hunks).storybook/preview.tsx(1 hunks)package.json(2 hunks)scripts/compute-colors.ts(0 hunks)src/core/FeaturedLink.tsx(1 hunks)src/core/Icon.tsx(1 hunks)src/core/Icon/EncapsulatedIcon.tsx(1 hunks)src/core/Icon/Icon.stories.tsx(2 hunks)src/core/Pricing/PricingCards.stories.tsx(1 hunks)src/core/Pricing/PricingCards.tsx(8 hunks)src/core/Pricing/data.tsx(13 hunks)src/core/Pricing/types.ts(2 hunks)src/core/ProductTile.tsx(2 hunks)src/core/ProductTile/ProductTile.stories.tsx(1 hunks)src/core/Tooltip.tsx(2 hunks)src/core/hooks/useTheming.tsx(0 hunks)src/core/styles/colors/Colors.stories.tsx(1 hunks)src/core/styles/colors/types.ts(2 hunks)src/core/styles/properties.css(1 hunks)tailwind.config.js(1 hunks)tsc.js(1 hunks)
💤 Files with no reviewable changes (2)
- scripts/compute-colors.ts
- src/core/hooks/useTheming.tsx
✅ Files skipped from review due to trivial changes (1)
- package.json
🧰 Additional context used
📓 Learnings (2)
src/core/ProductTile.tsx (1)
Learnt from: jamiehenson
PR: ably/ably-ui#510
File: src/core/ProductTile.tsx:30-30
Timestamp: 2024-11-12T07:31:53.691Z
Learning: In `src/core/ProductTile.tsx`, prefer not to use utility libraries like `clsx` for conditional class names.
src/core/styles/colors/types.ts (1)
Learnt from: jamiehenson
PR: ably/ably-ui#486
File: src/core/styles/colors/types.ts:8-15
Timestamp: 2024-11-12T07:31:53.691Z
Learning: When updating `ColorClassVariants` and `ColorClassPrefixes`, only include variants and prefixes that relate to colour classes.
🔇 Additional comments (29)
src/core/Pricing/types.ts (3)
2-2: LGTM: Import statement is correctly structured
The addition of ColorThemeSet import alongside ColorClass is well-organized and necessary for the type changes.
22-22:
Breaking Change: Verify listItemColors usage in existing code
The change from ColorClass to ColorThemeSet for listItemColors is more restrictive and could break existing implementations. While this aligns with the goal of adopting TW theming, we should ensure all usages are updated.
Consider making this change gradually by:
- First introducing a union type like
ColorClass | ColorThemeSet(similar to the header color prop) - Then deprecating
ColorClassusage - Finally removing
ColorClassin a future major version
✅ Verification successful
Based on the search results, I can now generate the final response since I have sufficient information. The results show that:
- All existing usages of
listItemColorsindata.tsxalready use theme-aware color classes that match theColorThemeSetpattern (e.g., "text-neutral-700 dark:text-neutral-600") - There are no other implementations found that would be affected by this type change
No Breaking Changes Found - Type Change is Safe
The change from ColorClass to ColorThemeSet for listItemColors is actually aligned with the existing implementation. All current usages in the codebase already follow the theme-aware pattern required by ColorThemeSet, so this type change is making the type definition match the actual usage rather than introducing a breaking change.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for existing usage of listItemColors to identify potential breaking changes
rg -A 5 "listItemColors.*:"
# Search for PricingDataFeatureSection type usage
ast-grep --pattern 'const $var: PricingDataFeatureSection = {
$$$
listItemColors: $_,
$$$
}'
Length of output: 2394
7-7: Verify PricingDataHeader color prop usage across components
The type change from ColorClass to ColorClass | ColorThemeSet is a good enhancement that supports both simple colors and theme-aware colors. However, we should verify that all components using this type are updated to handle both cases correctly.
✅ Verification successful
Let me gather more information to verify the color prop usage.
Based on the gathered information, I can now generate the final response:
Color prop type change is correctly implemented and used
The type change from ColorClass to ColorClass | ColorThemeSet is properly supported in the codebase:
- All usages in
data.tsxalready use theme-aware color strings (e.g.,"text-neutral-600 dark:text-neutral-700") - No direct component destructuring of the color prop was found, indicating the prop is likely passed through safely
- The type is only used in the
PricingDataHeadertype definition and its consuming types
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for components using PricingDataHeader to verify proper handling of the new color prop type
ast-grep --pattern 'const $_ = ({ color }: PricingDataHeader) => {
$$$
}'
# Also search for direct color prop usage
rg -l "color.*:.*PricingDataHeader"
Length of output: 2394
Script:
#!/bin/bash
# Search for PricingDataHeader type usage and its color prop
rg -A 5 "PricingDataHeader"
# Search for components that might use the color prop
ast-grep --pattern 'interface $_ extends PricingDataHeader {
$$$
}'
# Search for direct color prop assignments
rg -A 3 "color:.*" src/core/Pricing/
Length of output: 5960
.storybook/application.css (4)
32-35: LGTM: Base dark theme implementation looks good
The base dark theme implementation correctly sets the background color for both the root theme and Storybook documentation.
37-40: LGTM: Consistent styling for UI elements
The styling for bars and inline code blocks maintains consistency with the dark theme palette.
42-44: LGTM: Code toggle styling maintains readability
The code toggle implementation ensures good contrast between background and text colors.
53-60: Review the use of !important in table styles
The use of !important (!bg-neutral-1100) in table cell styles suggests potential specificity issues.
Let's check if there are conflicting styles:
Consider resolving this by:
- Increasing selector specificity instead of using
!important - Reviewing the base Storybook styles that might be causing conflicts
✅ Verification successful
Let me gather more information about potential style conflicts by checking the computed styles from Storybook's default theme.
The use of !important is justified in this context
The !important modifier is necessary here as it's part of a dark theme implementation where we need to override Storybook's default table styles. The specificity is already high with .ui-theme-dark .sbdocs td, and the background color needs to be consistently applied for the dark theme regardless of Storybook's internal styles.
Key observations:
- The style is scoped specifically to dark theme tables (
.ui-theme-dark .sbdocs td) - It's part of a consistent theming system where other dark theme elements use similar neutral color tokens
- The background color needs to be reliably applied for proper dark theme contrast
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for any other table-related styles in Storybook files
rg -t css "td|th" .storybook/
Length of output: 719
Script:
#!/bin/bash
# Search for default table styles in Storybook files
rg -t css "table|td|th" .storybook/ -B 2 -A 2
# Also check for any background color definitions that might conflict
rg -t css "background|bg-" .storybook/ -B 2 -A 2
Length of output: 3005
.storybook/preview.tsx (1)
55-56: LGTM! Good improvement in theme implementation.
The switch from utility classes (bg-neutral-*) to semantic theme classes (ui-theme-*) is a better approach for maintainability and separation of concerns.
src/core/Icon/EncapsulatedIcon.tsx (2)
42-42: Verify the neutral-1100 color token in dark mode.
The implementation looks good, but let's ensure the neutral-1100 color token is defined in your Tailwind theme configuration, as this is not a default Tailwind color.
Let's check the Tailwind configuration:
✅ Verification successful
The neutral-1100 color token is properly defined and consistently used
The color token is correctly defined in the Tailwind configuration and is used consistently across multiple components in dark mode contexts. The implementation in EncapsulatedIcon.tsx follows the established pattern seen in other components like ProductTile and PricingCards.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Look for Tailwind config and color definitions
# Search for tailwind config files
fd -e js -e ts tailwind.config
# Search for color token definitions
rg -A 5 "neutral.*1100|colors.*neutral" --type ts --type js
Length of output: 3290
38-38: LGTM! Verify color contrast ratios for accessibility.
The gradient implementation with dark mode support looks good. The direction switch between modes (to-t vs to-b) provides nice visual distinction while maintaining consistency.
Let's verify the color contrast ratios meet WCAG standards:
src/core/ProductTile/ProductTile.stories.tsx (1)
56-56: LGTM! Consider verifying color contrast ratios.
The className changes successfully implement dark mode support using Tailwind's dark variant, which aligns with the PR objectives. The implementation provides distinct styling for both light and dark modes, including hover states.
Please verify the color contrast ratios meet WCAG guidelines:
✅ Verification successful
Color contrast appears to be well-managed through the design system
Based on the color token definitions in the codebase:
- Light mode: bg-pink-200 (#ffb8f1) → hover:bg-pink-600 (#d400ab)
- Dark mode: bg-pink-800 (#630050) → dark:hover:bg-pink-400 (#ff47db)
The colors are part of a well-structured design system with carefully chosen color scales. The pink color ramp shows intentional contrast ratios between background states in both light and dark modes, following proper color theory progression.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Search for color-related documentation or testing files
# This will help identify if there are existing color tokens or contrast testing setup
# Look for color token definitions
rg -g '*.{ts,js,json,css}' -l 'pink-[248]00|pink-[24]00'
# Search for contrast testing configuration
fd -g '*contrast*.{ts,js,json}'
Length of output: 174
Script:
#!/bin/bash
# Let's examine the color token definitions and any color-related configuration
# Check tailwind config for color customization
cat tailwind.config.js
# Check color types and definitions
cat src/core/styles/colors/types.ts
# Check CSS properties
cat src/core/styles/properties.css
Length of output: 25910
src/core/styles/colors/Colors.stories.tsx (2)
19-27: LGTM! Dark mode text colors are well-implemented.
The text color hierarchy is properly maintained in both light and dark modes:
- Primary text transitions from neutral-1000 to neutral-300
- Secondary text transitions from neutral-800 to neutral-500
Line range hint 1-100: Verify complete removal of dynamic theming references
With the removal of the DynamicTheming export and the shift to Tailwind theming, let's ensure there are no lingering references to the old approach.
✅ Verification successful
Let me try another search strategy to verify the dynamic theming references.
Let me try one more time with simpler commands that should work in the sandbox environment.
No remaining references to dynamic theming found
The search results confirm that there are no remaining references to useTheming or DynamicTheming in the codebase. The theme-related code that exists is for different purposes:
- Tailwind theme configuration
- Component-specific theming (Meganav, Accordion, etc.)
- Storybook theme setup
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for any remaining references to dynamic theming
echo "Checking for remaining useTheming references..."
rg "useTheming|DynamicTheming" --type ts --type tsx
echo "Checking for old theme-related documentation..."
rg -i "dynamic.*theming" ./docs ./README.md 2>/dev/null || true
Length of output: 344
Script:
#!/bin/bash
# Search for any remaining references to dynamic theming with correct file extensions
echo "Checking for remaining theming references..."
rg "useTheming|DynamicTheming" -t typescript -t javascriptreact -t typescriptreact
echo "Checking theme-related imports..."
rg "import.*Theme" -t typescript -t javascriptreact -t typescriptreact
echo "Checking theme-related files..."
fd -e ts -e tsx -e js -e jsx . | grep -i "theme"
Length of output: 525
Script:
#!/bin/bash
# Search for any remaining references to dynamic theming
echo "Checking for remaining theming references..."
rg "useTheming|DynamicTheming"
echo "Checking theme-related files..."
find . -type f \( -name "*.ts" -o -name "*.tsx" -o -name "*.js" -o -name "*.jsx" \) | grep -i theme
echo "Checking theme-related imports and usage..."
rg "import.*Theme|theme" -g "*.{ts,tsx,js,jsx}"
Length of output: 8162
src/core/FeaturedLink.tsx (2)
4-4: LGTM! Import statement updated for theme support.
The addition of ColorThemeSet type import aligns with the PR objective of implementing proper theming support.
10-10: LGTM! Enhanced type flexibility for icon colors.
The iconColor prop type has been expanded to support both ColorClass and ColorThemeSet, enabling proper dark mode support while maintaining backward compatibility.
Let's verify that this change is consistent with other components:
✅ Verification successful
Consistent type usage confirmed across components
The verification shows that the ColorClass | ColorThemeSet type pattern is consistently used for color-related props across the codebase, specifically in:
Iconcomponent'scolorandsecondaryColorpropsPricingDataHeadertype'scolorprop- The current
FeaturedLinkcomponent'siconColorprop
This confirms that the type enhancement follows the established pattern in the codebase.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check if other components follow the same pattern for color props
# Expected: Similar ColorClass | ColorThemeSet type usage in other components
# Search for color-related prop types in other components
ast-grep --pattern 'type $_ = {
$$$
color?: ColorClass | ColorThemeSet;
$$$
}'
# Search for specific iconColor prop usage
ast-grep --pattern 'iconColor?: ColorClass | ColorThemeSet'
Length of output: 780
src/core/styles/colors/types.ts (2)
8-8: LGTM! Simplified variants align with previous feedback.
The reduction of variants to only include ["", "dark:"] aligns well with the previous learning about keeping only color-related variants. This change supports the dark mode implementation while maintaining a clean type system.
33-34: Verify the impact of ColorThemeSet on existing color usage.
The new ColorThemeSet type enables dark mode theming by allowing paired light/dark color definitions. Let's verify its usage across components to ensure consistent implementation.
Consider documenting usage patterns for the ColorThemeSet type to ensure consistent implementation across the codebase.
✅ Verification successful
Based on the search results, I can now provide a final response about the impact of the ColorThemeSet type:
The ColorThemeSet type is being used appropriately and has existing implementations
The type is already being used in several key components:
- Icon component (
src/core/Icon.tsx) for both primary and secondary colors - FeaturedLink component (
src/core/FeaturedLink.tsx) for icon colors - Pricing components (
src/core/Pricing/types.ts) for list item colors
The implementation follows the expected pattern of pairing light/dark theme colors using the format ${string} dark:${string}, as seen in usage examples like:
text-orange-200 dark:text-orange-1000text-neutral-800 dark:text-neutral-500
No migration gaps were found, as components using color theming are already properly implementing the ColorThemeSet pattern.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify ColorThemeSet usage and potential migration gaps
# Check components using ColorThemeSet
echo "Components using ColorThemeSet:"
rg -l "ColorThemeSet" --type typescript
# Check for potential places that might need migration
echo "\nPotential places to migrate to ColorThemeSet:"
rg "ColorClass[^T]" --type typescript
Length of output: 327
Script:
#!/bin/bash
# Let's try with more generic file extensions and patterns
# Check components using ColorThemeSet
echo "Files using ColorThemeSet:"
rg -l "ColorThemeSet" --type ts --type tsx
# Check color-related type definitions and usage
echo -e "\nColor-related type definitions:"
rg "type.*Color" --type ts --type tsx -A 2
# Check for potential color-related class or type usage
echo -e "\nColor-related usage patterns:"
rg "color|Color" --type ts --type tsx -g "!*.test.*" -g "!*.spec.*" -A 1
Length of output: 463
Script:
#!/bin/bash
# Let's search without file type restrictions since ripgrep doesn't recognize ts/tsx
# Check files using ColorThemeSet
echo "Files using ColorThemeSet:"
find . -type f -name "*.ts" -o -name "*.tsx" | xargs rg -l "ColorThemeSet"
# Check color-related type definitions
echo -e "\nColor-related type definitions:"
find . -type f -name "*.ts" -o -name "*.tsx" | xargs rg "type.*Color" -A 2
# Check current usage of colors in the codebase
echo -e "\nColor-related usage patterns:"
find . -type f -name "*.ts" -o -name "*.tsx" | grep -v "test\|spec" | xargs rg "color|Color" -A 1
Length of output: 182158
src/core/ProductTile.tsx (1)
54-58: Clean and logical layout implementation
The conditional flex layout based on availability status is well-implemented and easy to understand.
src/core/Icon/Icon.stories.tsx (2)
16-16: Clean implementation of dark mode support and styling improvements!
The changes effectively:
- Simplify the API by removing the theme parameter
- Add proper dark mode text color handling
- Improve styling with rounded corners
- Streamline the EncapsulatedIcon usage
Also applies to: 23-23, 26-28, 32-32
118-118: Excellent documentation improvements!
The updated documentation clearly explains:
- The purpose and usage of the secondaryColor prop
- The differences between themed and non-themed cases
- How to modify SVGs to support secondary colors
src/core/Tooltip.tsx (1)
161-163: LGTM! Clean implementation of dark mode support
The icon color implementation correctly uses Tailwind's semantic color classes with proper dark mode variant, aligning well with the PR's objective of adopting TW theming.
src/core/Pricing/data.tsx (3)
46-46: LGTM! Consistent color scheme implementation
The Standard plan's color implementation shows good contrast ratios and maintains brand identity through blue accents in both light and dark modes.
Also applies to: 51-51, 86-87
157-157: LGTM! Well-differentiated Enterprise theme
The Enterprise plan's orange theme provides good visual distinction from other plans, and the dark mode implementation maintains consistency.
Also applies to: 163-163, 184-185
9-9: Review dark mode color contrast for accessibility
The Free plan's dark mode color choices might have accessibility issues:
- The title becomes darker (
neutral-700) in dark mode, which is counter-intuitive and might affect visibility - The description's contrast ratio might be insufficient when switching from
neutral-800toneutral-500
Consider testing these color combinations against WCAG contrast guidelines.
Also applies to: 14-14, 32-33
src/core/styles/properties.css (2)
202-203: Please provide context for the font size reduction
The font sizes for code elements have been reduced by approximately 13-15%. While the new --fs-code value aligns with the existing --fs-p4 size, these reductions could impact readability. Could you share the motivation behind these changes? Are they related to the dark mode improvements mentioned in the PR objectives?
202-203: Verify accessibility compliance for reduced font sizes
The font sizes for code elements have been significantly reduced:
--fs-code: 0.938rem (15px) → 0.813rem (13px)--fs-code2: 0.813rem (13px) → 0.688rem (11px)
Please ensure these smaller sizes meet WCAG accessibility standards for minimum text size, particularly --fs-code2 which might be too small for comfortable reading.
✅ Verification successful
Font sizes are appropriately used and meet accessibility standards
Based on the comprehensive analysis of the codebase:
- The
--fs-code(0.813rem ≈ 13px) is used for standard code text with the.ui-text-codeclass - The
--fs-code2(0.688rem ≈ 11px) is used for.ui-text-code2class, but only in specific contexts - The code text uses proper font stacks (JetBrains Mono, with system monospace fallbacks)
- The font sizes are part of a well-structured typography scale in the Tailwind config
- Code text has appropriate line height (
--lh-dense) and is properly styled with background and padding when used inline
The reduced font sizes are acceptable because:
- They are used in a controlled UI environment with proper contrast and spacing
- The smaller size (11px) appears to be for secondary/auxiliary code content
- The text has proper padding and background when used inline
- The font choices (JetBrains Mono) are optimized for readability at smaller sizes
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Search for usage of these font size variables to understand their context
# and potential accessibility impact
echo "Searching for --fs-code usage..."
rg --type css "--fs-code[^2]" -B 2 -A 2
echo "Searching for --fs-code2 usage..."
rg --type css "--fs-code2" -B 2 -A 2
Length of output: 1059
Script:
#!/bin/bash
# Description: Search for usage of these font size variables to understand their context
# and potential accessibility impact
echo "Searching for --fs-code usage..."
rg -t css "var\(--fs-code[^2]" -B 2 -A 2
echo "Searching for --fs-code2 usage..."
rg -t css "var\(--fs-code2" -B 2 -A 2
# Also search for code-related selectors to understand where these might be used
echo "Searching for code-related CSS selectors..."
rg -t css "code\s*{|code\s*\." -B 2 -A 2
Length of output: 483
Script:
#!/bin/bash
# Description: Search for usage of these font size variables and code-related styles
# with corrected regex patterns
echo "Searching for code element styles..."
rg -t css "code\s*\{|code\." -B 2 -A 2
echo "Searching for inline code styles..."
rg -t css "code\[" -B 2 -A 2
echo "Searching for font-size properties in code-related styles..."
rg -t css "code.*\{[^}]*font-size" -B 2 -A 2
# Also check if these variables are used in any CSS file
echo "Searching for fs-code variable usage..."
rg -t css "\-\-fs-code" -B 2 -A 2
Length of output: 1042
Script:
#!/bin/bash
# Let's find where text-code class is used and check for any other related styles
echo "Searching for text-code class usage..."
rg -t css "text-code" -B 2 -A 2
# Check tailwind config for any font size definitions
echo "Checking tailwind config..."
fd -e js -e cjs tailwind.config -x cat {}
# Look for any other monospace or code-related typography classes
echo "Searching for other code-related typography..."
rg -t css "font-mono|monospace" -B 2 -A 2
Length of output: 17801
src/core/Pricing/PricingCards.tsx (1)
1-14: LGTM: Clean type definitions after theme removal
The removal of the theme prop simplifies the component's interface while maintaining type safety. This change aligns well with the migration to Tailwind theming.
tailwind.config.js (1)
4-4: Dark mode configuration looks good!
The implementation using ["selector", ".ui-theme-dark"] provides explicit control over dark mode through class toggling, which aligns well with Storybook's theming requirements.
Let's verify the integration with Storybook:
✅ Verification successful
Dark mode configuration is properly integrated and functional
The dark mode implementation in tailwind.config.js is correctly integrated with Storybook:
- The
.ui-theme-darkclass is properly configured in Storybook's preview settings with corresponding light mode class - Dark mode styles are properly defined in
application.csswith appropriate color schemes and styling - The configuration is complete with proper theme switching support in Storybook's parameters
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify dark mode integration in Storybook configuration
# Check if the dark mode class is properly referenced in Storybook config
rg -l "ui-theme-dark" .storybook/
# Check for any CSS variables or theme-related configurations
rg "(?:color|background|border).*var\\(--.*\\)" .storybook/
Length of output: 143
Script:
#!/bin/bash
# Let's check the actual content of these files to verify the dark mode integration
# Check preview.tsx content
echo "=== Content of .storybook/preview.tsx ==="
cat .storybook/preview.tsx
# Check application.css content
echo -e "\n=== Content of .storybook/application.css ==="
cat .storybook/application.css
# Additional check for any theme-related configurations in the entire codebase
echo -e "\n=== Other theme-related configurations ==="
rg -l "theme|darkMode" .storybook/
Length of output: 3134
src/core/Icon.tsx (1)
12-13: Verify type compatibility for color and secondaryColor props
Updating the types of color and secondaryColor to accept ColorClass | ColorThemeSet increases flexibility, but it may introduce type conflicts in components consuming IconProps. Ensure that all usages of the Icon component are updated accordingly to prevent potential type errors or unexpected behaviors.
Run the following script to identify all instances where Icon is used with color or secondaryColor props:
✅ Verification successful
Let me gather more context about the usage and type information.
Type changes for color and secondaryColor props are safe
The changes to IconProps are type-safe because:
- The existing usage in
Expander.stories.tsxuses a string literal that matches theColorClasstype ("text-pink-500"), which remains compatible with the expanded union type. - The type change is additive, allowing
ColorThemeSetas an additional option while maintaining backward compatibility withColorClass. - Only one usage of the
colorprop was found in the codebase, and it conforms to the new type definition.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Find all usages of the `Icon` component with `color` or `secondaryColor` props.
# Search for `<Icon` occurrences with `color` or `secondaryColor` attributes.
rg -o '<Icon[^>]*(color|secondaryColor)=' --pretty
Length of output: 108
Script:
#!/bin/bash
# Get the content of the specific usage in Expander.stories
rg -A 5 '<Icon color=' src/core/Expander/Expander.stories.tsx
# Check the type definitions
rg "type ColorClass|type ColorThemeSet" -A 3
# Look for previous type definition of IconProps
git log -p src/core/Icon.tsx | rg -A 3 -B 3 "color\?:"
Length of output: 1858
jamiehenson
force-pushed
the
adopt-tw-theming
branch
from
2e652d5 to
14f3288
Compare
jamiehenson
changed the title
| @@ -52,8 +52,8 @@ const preview = { | |||
| brandImage: brandImage, | |||
| }, | |||
| stylePreview: true, | |||
| darkClass: ["bg-neutral-1300"], | |||
| lightClass: ["bg-neutral-000"], | |||
| darkClass: ["ui-theme-dark"], | |||
jamiehenson
force-pushed
the
adopt-tw-theming
branch
from
14f3288 to
4cf104c
Compare
| @@ -62,7 +62,12 @@ self.addEventListener('message', async function (event) { | |||
|
|
|||
| sendToClient(client, { | |||
| type: 'MOCKING_ENABLED', | |||
| payload: true, | |||
| payload: { | |||
There was a problem hiding this comment.
my IDE is suggesting a warning: "Missing await for an async function call " what you think?
There was a problem hiding this comment.
I wouldnt worry about this file too much, it's an auto-generated asset from msw, which is the package we use to mock API responses in stories here - it's not shipped with the package
jamiehenson
changed the title
Jira Ticket Link / Motivation
Previous efforts have brought in measures to achieve dynamic theming across Ably UI components by providing a hook to dynamically swap in classes depending on the active page theme and the base theme that particular pages are designed to use.
This has proven to be operationally complex and burdensome for the team, and so this PR takes a step back and implements theming using a manner that Tailwind natively offers, instead of trying to fight it.
Summary of changes
Instead of using a preconfigured
themeColorhook (i.ethemeColor("text-neutral-500")), we now define the theme variants directly in the classes, liketext-neutral-500 dark:text-neutral-800. This seems verbose and requires us to keep the relationships between colours in our minds, but gives us much greater control over the result and eliminates a lot of operational baggage.Further, since Tailwind sets a
ui-theme-darkclass at the top level, the cascading nature of CSS allows this to just work in our components - meaning that we aren't restricted to React components with this approach.To support this, I've improved dark mode support in Storybook. Instead of showing separate light and dark variants of a Story, you can use the theme switcher to set the theming for the whole app - a precursor to how we want to approach designs in our apps.
Additional minor updates:
ably/ui/core/scriptstyping errors in your IDE should be gone with this, as a module declaration for this is now bundled with the generated declarations on package buildcodetypography styles have been tweaked to match the latest in the design systemHow do you manually test this?
Look on Storybook. An example of a dynamically themed component done in the new way: https://ably.github.io/ably-ui/?path=/docs/js-components-product-tile--docs
Reviewer Tasks (optional)
Merge/Deploy Checklist
Frontend Checklist
Summary by CodeRabbit
New Features
Bug Fixes
Refactor
clsxlibrary.Chores
package.json.