Conversation
* feat: add ds-tokens and semantic tokens * fix: resolve build error * feat: update core to semantic tokens * feat: validate tokens * feat: add dark mode toggle * fix: color update * fix: update maps to preserve token values * fix: update heading values * fix: update tokens and dark mode values * fix: update token values * fix: update semantic token mapping from loop * fix: token cleanup * fix: remove dark mode temp files * fix: update color token pairings * fix: update ds tokens dark mode dev version * fix(sage-alert): update colors to match pine alert * fix(sage-alert): update alert close button color * fix: update subtle button dark mode colors * fix: add latest ds tokens * fix: remove console warnings * fix: update buttons hover colors * fix: remove problematic @import from _ds_tokens.scss - Reverted automatic Pine tokens import that broke Vite builds - The ~@ prefix is webpack-specific and incompatible with Vite - Tokens are now documented as requiring app-level loading (CDN or import) - Added Pine tokens import to sage-lib docs specifically (webpack env) - Bumped version to 6.2.25-dev.9 * fix(button): move dark theme override to end of file for CSS cascade - Move [data-theme="dark"] block to Theme Overrides section at EOF - Ensures dark theme styles properly override generated button styles - Add comment to prevent future regressions Bump sage-assets to 6.2.25-dev.11 * fix: update to latest packs and dropdown semantic border * style: update to use semantic tokens grey black white * fix: update semantic tokens names * fix: update to semantic values * fix: add lock file * fix: update to ds tokens 1.0.1 * fix: remove hard coded dark mode attr * fix: resolve linter errors * style: oop fix * style: oop fix * style: oop fix * fix: oop fix * fix: oop fix * fix: remove ds tokens dependancy * fix: align alert switch and tooltip to pine styles * fix: align badge input popover property and form element error message * fix: update disabled border * fix: added border's to modal like things * fix: lint error fix * fix: update tokens * fix: remove yarn dependancy * fix: update hero custom bg example * fix: updated pagination * style: updated alerts and banner to core tokens * fix: update dark mode error state * fix: remove border and update lang * fix: update alert icon and text colors * fix: update alert and button color * fix: update disabled select icon color
Remove CodeQL workflow - migrating to Aikido
|
This pull request sets up GitHub code scanning for this repository. Once the scans have completed and the checks have passed, the analysis results for this pull request branch will appear on this overview. Once you merge this pull request, the 'Security' tab will show more code scanning analysis results (for example, for the default branch). Depending on your configuration and choice of analysis tool, future pull requests will be annotated with code scanning analysis results. For more information about GitHub code scanning, check out the documentation. |
|
@coderabbitai review this PR |
|
@QuintonJason, I'll review the changes in this PR for you. ✅ Actions performedReview triggered.
|
📝 WalkthroughWalkthroughThis pull request removes GitHub Actions workflow files for CodeQL and Dependency Review, adds a dark mode toggle feature to the documentation site with accompanying JavaScript implementation, and performs a comprehensive migration of color tokens across the codebase. The migration replaces Sage color helper functions with Pine design system CSS custom properties throughout component stylesheets. New SCSS helper functions are introduced to map semantic color keys to CSS variables, and dark theme overrides are added across multiple components. Configuration updates include modified PostCSS settings and stylesheet imports. Documentation and README files are updated to reflect the design system changes. Pre-merge checks and finishing touches❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (1 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 18
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (5)
packages/sage-assets/lib/stylesheets/components/_hero.scss (1)
84-109: Ensure custom background colors don't create accessibility violations.The hardcoded dark grey colors (
grey-950for title,grey-900for body) will fail WCAG AA contrast requirements (4.5:1 minimum for normal text) if users provide dark custom backgrounds. The component currently accepts any color without validation, allowing users to create inaccessible combinations unintentionally.Consider adding:
- Input validation or documentation constraining custom backgrounds to light colors only
- A contrast validator that warns developers about insufficient contrast ratios
- Conditional color logic that lightens text when background lightness is below a threshold
packages/sage-assets/lib/stylesheets/components/_stat_box.scss (1)
103-118: Legend colors still use compile-time Sage tokens, breaking dark mode support.While the rest of the file has been migrated to Pine CSS variables, the legend color loop still computes colors at SCSS compile-time using
sage-color(). These hardcoded values won't adapt to theme changes, creating an inconsistency where legend markers may have poor contrast or incorrect semantics in dark mode.Consider migrating these to Pine CSS variables or semantic color tokens that respond to theme changes.
Example refactor approach:
-@each $-color-name, $-color-sliders in $sage-colors { - .sage-stat-box__title--legend-#{"" + $-color-name} { - @if ($-color-name == blue) { - --legend-color: #{sage-color($-color-name, 600)}; - } - @else if ($-color-name == green) { - --legend-color: #{sage-color($-color-name, 600)}; - } - @else if ($-color-name == mercury) { - --legend-color: #{sage-color($-color-name, 500)}; - } - @else { - --legend-color: #{sage-color($-color-name, 500)}; - } - } -} +@each $-color-name in (blue, green, red, yellow, orange, purple, mercury) { + .sage-stat-box__title--legend-#{$-color-name} { + --legend-color: var(--pine-color-#{$-color-name}-500); + } +}Note: Verify that corresponding Pine color variables exist for all required color names.
packages/sage-assets/lib/stylesheets/components/_progress_bar.scss (1)
69-72: Likely typo in CSS variable name for font weight
font-weight: var(---pine-font-weight-medium);uses three leading dashes, which is inconsistent with other--pine-*variables and likely doesn’t resolve to the intended token.Consider correcting it:
-.sage-progress-bar__percent { - font-weight: var(---pine-font-weight-medium); +.sage-progress-bar__percent { + font-weight: var(--pine-font-weight-medium);docs/app/views/application/_app_head_content.html.erb (1)
21-22: Pin the Pine DS Core JavaScript modules to a specific version.The CSS stylesheet on line 12 uses a pinned version (
@kajabi-ui/styles@1.0.2), but these JavaScript modules on lines 21-22 still use@latest. This creates version drift and unpredictable behavior. Using @latest is dangerous because major versions usually come with breaking changes.Update both scripts to match the CSS versioning strategy by replacing
@latestwith a specific version number that was tested with your CSS stylesheet.packages/sage-assets/lib/stylesheets/components/_assistant.scss (1)
49-49: Use SCSS'salpha()function instead ofrgba()for CSS custom properties.Line 49 uses
rgba($-assistant-search-bg-color, $-assistant-search-bg-opacity)where$-assistant-search-bg-coloris set tovar(--pine-color-grey-200). SCSS'srgba()function does not work with CSS custom properties at compile time. Replace withalpha($-assistant-search-bg-color, $-assistant-search-bg-opacity)which properly supportsvar()references.
🧹 Nitpick comments (15)
docs/postcss.config.js (1)
11-27: Move module imports to the top of the file.The
require('path')andrequire('fs')statements on lines 17-18 are currently inside the catch block. While this works, it's a best practice violation that makes dependencies less clear and code harder to maintain.Apply this diff to move the imports to the top of the module:
+const path = require('path'); +const fs = require('fs'); + module.exports = { plugins: [ require('postcss-import')({ // Resolve imports from node_modules (handles ~ alias) resolve(id, basedir) { // Strip ~ alias if present (common webpack syntax) if (id.startsWith('~')) { id = id.slice(1); } // Try to resolve as an npm package if (!id.startsWith('.') && !id.startsWith('/')) { try { return require.resolve(id); } catch (e) { // If require.resolve fails, try relative to basedir - const path = require('path'); - const fs = require('fs'); const possiblePaths = [ path.resolve(basedir || __dirname, 'node_modules', id), path.resolve(basedir || __dirname, '..', 'node_modules', id), ]; for (const p of possiblePaths) { if (fs.existsSync(p)) return p; } } } // Fall back to default resolver for relative paths // Return id to let webpack resolve it (webpack handles node_modules better) return id; } }),packages/sage-assets/lib/stylesheets/components/_billboard.scss (1)
9-9: Consider using CSS variables for text color to support theming.The background color now uses a CSS variable for theming, but the text color remains hardcoded as
#fff. For full dark mode support and to ensure adequate contrast across all theme variations, consider migrating the text color to a CSS variable as well.-$-billboard-text-color: #fff; +$-billboard-text-color: var(--pine-color-on-primary);Note: This assumes a corresponding
--pine-color-on-primaryvariable exists or will be defined as part of the theming system.packages/sage-assets/lib/stylesheets/components/_dot.scss (1)
8-8: Consider adding a hard-coded fallback to the CSS variable.The CSS variable
var(--pine-color-brand)lacks a hard-coded fallback. If this variable is undefined at runtime, dots will render without a background color.Apply this diff to add a fallback value:
-$-sage-dot-color: var(--pine-color-brand); +$-sage-dot-color: var(--pine-color-brand, #000);Alternatively, if a brand color hex value is available as a constant in the design system, use that instead of
#000.packages/sage-react/CHANGELOG.md (1)
6-12: Dev changelog entry follows existing conventions; double‑check compare range.The new
6.2.25-dev.19section is formatted consistently. Optional: confirm the compare link should start fromv6.2.24(rather thanv6.2.25) so the diff range matches how you intend to represent stable vs dev changes.packages/sage-assets/lib/stylesheets/tokens/_type_specs.scss (1)
197-205: Centralized heading color in type placeholders is good; align the comment with the implementation.Applying
var(--pine-color-text-strong)in%t-sage-heading-*ensures consistent heading color across components and should be safely overridden where a component sets its own color later. The inline comment still refers to usingsage-text-color()though the code now usesvar(--pine-color-text-strong)directly; consider either calling the helper (if preferred) or updating the comment to avoid confusion.packages/sage-assets/lib/stylesheets/components/_form_select.scss (1)
55-76: Form select color token migration looks good; consider disabled arrow styling
- Using
sage-text-color(body)for options andsage-field-color(default)for the selected value is the right move toward the new semantic helpers.- Updating the arrow to
var(--pine-color-text-muted)/var(--pine-color-text-tertiary)aligns it with the Pine text tokens.One nuance: in the disabled state, only the field/placeholder colors change, so the arrow icon now keeps the non‑disabled tokens. If design expects the arrow to appear visually disabled as well, you might optionally set the arrow (and/or its
::before) color under&:disabledto a disabled semantic likevar(--pine-color-text-disabled)for consistency.Also applies to: 95-106, 138-145
docs/lib/sage-frontend/stylesheets/docs/_quick_links.scss (1)
7-16: Consider completing the CSS variable migration.Lines 7 and 16 have been migrated to CSS variables, but lines 8-9 still use the old
sage-color()function. For consistency with the broader theming refactor, consider migrating these as well:-$-quick-links-color-focus: sage-color(primary, 300); -$-quick-links-color-background: sage-color(primary, 200); +$-quick-links-color-focus: var(--pine-color-primary-hover); +$-quick-links-color-background: var(--pine-color-primary);Note: If these colors are used in
rgba()operations (lines 47, 56), you may need to usesage-color-hex()instead to maintain compatibility.docs/lib/sage-frontend/stylesheets/docs/_colors.scss (1)
47-106: Consider the refactored color generation logic once pipeline is fixed.The refactored color block generation with dynamic contrast logic based on
lightness()is a good improvement for readability. However, the implementation depends onsage-color-hex()returning valid color values for all color/value combinations in$sage-colors.Once the pipeline failure is resolved, verify that all color values used in the lightness calculations exist and return valid hex colors.
docs/lib/sage-frontend/stylesheets/docs/_specs.scss (2)
10-11: Consider more appropriate border token for table headers.The header uses
var(--pine-color-border-hover)for the bottom border. Since this is a static table border (not a hover state), consider using a token like--pine-color-borderor--pine-color-border-defaultif available for better semantic clarity.
18-18: Consider more appropriate border token for table data cells.The border uses
var(--pine-color-border-disabled)which semantically suggests a disabled state. For regular table cell borders, consider using--pine-color-borderor a similar neutral token if available.docs/app/views/application/_assistant.html.erb (1)
25-25: Consider using a more intuitive icon for dark mode toggle.The "flash" icon may not be immediately recognizable as a dark mode toggle. Consider using icons like "moon", "sun", or "contrast" that more clearly convey the dark mode functionality to users.
packages/sage-assets/lib/stylesheets/components/_panel.scss (1)
49-59: Consider using semantic tokens consistently for divider styling.Line 49 uses a raw color token (
--pine-color-grey-200) while Lines 57 and 59 use semantic tokens. Additionally, Line 57 uses--pine-color-border-disabledfor text color, which seems semantically mismatched—a--pine-color-text-*variant would be more appropriate for thecolorproperty.docs/lib/sage-frontend/javascript/docs/dark-mode-toggle.js (2)
18-35: Consider storing matchMedia reference for potential cleanup.The
matchMedialistener is created inline and cannot be removed later. While this is likely fine for a singleton pattern, if the toggle could be re-initialized, this would create duplicate listeners.init() { // Set initial theme from localStorage or system preference const savedTheme = localStorage.getItem(THEME_KEY); - const prefersDark = window.matchMedia('(prefers-color-scheme: dark)').matches; + this.mediaQuery = window.matchMedia('(prefers-color-scheme: dark)'); + const prefersDark = this.mediaQuery.matches; const initialTheme = savedTheme || (prefersDark ? THEME_DARK : THEME_LIGHT); this.setTheme(initialTheme, false); // Listen for toggle clicks this.toggle.addEventListener('click', () => this.handleToggle()); // Listen for system theme changes - window.matchMedia('(prefers-color-scheme: dark)').addEventListener('change', (e) => { + this.mediaQuery.addEventListener('change', (e) => { if (!localStorage.getItem(THEME_KEY)) { this.setTheme(e.matches ? THEME_DARK : THEME_LIGHT, false); } }); }
59-62: Wrap localStorage access in try-catch for private browsing compatibility.In some browsers' private/incognito modes,
localStorageaccess can throw exceptions. Wrapping in try-catch prevents runtime errors.// Save to localStorage if requested if (save) { - localStorage.setItem(THEME_KEY, theme); + try { + localStorage.setItem(THEME_KEY, theme); + } catch (e) { + // localStorage may be unavailable in private browsing mode + } }Similarly, consider wrapping
localStorage.getItem()calls in Lines 20 and 31.packages/sage-assets/lib/stylesheets/components/_dropdown.scss (1)
284-318: Hardcoded white values in scroll shadow gradients.Lines 284-309 use literal
whiteandrgba(255, 255, 255, 0)values for the scroll shadow effect. While the dark mode override (Lines 315-318) replaces the entire background, this approach loses the scroll shadow effect in dark mode. Consider parameterizing the gradient colors for full dark mode support.
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (2)
docs/Gemfile.lockis excluded by!**/*.lockyarn.lockis excluded by!**/yarn.lock,!**/*.lock
📒 Files selected for processing (103)
.github/workflows/codeql.yml(0 hunks).github/workflows/dependency-review.yml(0 hunks)CHANGELOG.md(1 hunks)docs/CHANGELOG.md(1 hunks)docs/app/views/application/_app_head_content.html.erb(1 hunks)docs/app/views/application/_assistant.html.erb(1 hunks)docs/lib/sage-frontend/javascript/docs/dark-mode-toggle.js(1 hunks)docs/lib/sage-frontend/javascript/docs/index.js(1 hunks)docs/lib/sage-frontend/stylesheets/docs/_assistant.scss(1 hunks)docs/lib/sage-frontend/stylesheets/docs/_colors.scss(1 hunks)docs/lib/sage-frontend/stylesheets/docs/_quick_links.scss(2 hunks)docs/lib/sage-frontend/stylesheets/docs/_specs.scss(1 hunks)docs/lib/sage-frontend/stylesheets/docs/_status_key.scss(1 hunks)docs/lib/sage-frontend/stylesheets/docs/_status_table.scss(1 hunks)docs/lib/sage-frontend/stylesheets/docs/_text.scss(1 hunks)docs/lib/sage_rails/lib/sage_rails/version.rb(1 hunks)docs/package.json(2 hunks)docs/postcss.config.js(1 hunks)lerna.json(1 hunks)packages/sage-assets/CHANGELOG.md(1 hunks)packages/sage-assets/README.md(1 hunks)packages/sage-assets/lib/stylesheets/components/_alert.scss(5 hunks)packages/sage-assets/lib/stylesheets/components/_assistant.scss(2 hunks)packages/sage-assets/lib/stylesheets/components/_badge.scss(2 hunks)packages/sage-assets/lib/stylesheets/components/_banner.scss(1 hunks)packages/sage-assets/lib/stylesheets/components/_billboard.scss(1 hunks)packages/sage-assets/lib/stylesheets/components/_breadcrumbs.scss(3 hunks)packages/sage-assets/lib/stylesheets/components/_button.scss(8 hunks)packages/sage-assets/lib/stylesheets/components/_card.scss(4 hunks)packages/sage-assets/lib/stylesheets/components/_card_highlight.scss(1 hunks)packages/sage-assets/lib/stylesheets/components/_carousel.scss(1 hunks)packages/sage-assets/lib/stylesheets/components/_catalog_item.scss(2 hunks)packages/sage-assets/lib/stylesheets/components/_chart_legend.scss(1 hunks)packages/sage-assets/lib/stylesheets/components/_chart_summary.scss(1 hunks)packages/sage-assets/lib/stylesheets/components/_choice.scss(7 hunks)packages/sage-assets/lib/stylesheets/components/_data_card.scss(2 hunks)packages/sage-assets/lib/stylesheets/components/_description.scss(1 hunks)packages/sage-assets/lib/stylesheets/components/_divider.scss(1 hunks)packages/sage-assets/lib/stylesheets/components/_dot.scss(1 hunks)packages/sage-assets/lib/stylesheets/components/_dropdown.scss(11 hunks)packages/sage-assets/lib/stylesheets/components/_dynamic_select.scss(1 hunks)packages/sage-assets/lib/stylesheets/components/_empty_state.scss(2 hunks)packages/sage-assets/lib/stylesheets/components/_expandable_card.scss(1 hunks)packages/sage-assets/lib/stylesheets/components/_feature_toggle.scss(1 hunks)packages/sage-assets/lib/stylesheets/components/_form_input.scss(2 hunks)packages/sage-assets/lib/stylesheets/components/_form_section.scss(1 hunks)packages/sage-assets/lib/stylesheets/components/_form_select.scss(3 hunks)packages/sage-assets/lib/stylesheets/components/_hero.scss(6 hunks)packages/sage-assets/lib/stylesheets/components/_hint.scss(1 hunks)packages/sage-assets/lib/stylesheets/components/_icon.scss(1 hunks)packages/sage-assets/lib/stylesheets/components/_icon_list.scss(1 hunks)packages/sage-assets/lib/stylesheets/components/_indicator.scss(2 hunks)packages/sage-assets/lib/stylesheets/components/_input_group.scss(3 hunks)packages/sage-assets/lib/stylesheets/components/_label.scss(1 hunks)packages/sage-assets/lib/stylesheets/components/_link.scss(6 hunks)packages/sage-assets/lib/stylesheets/components/_list.scss(2 hunks)packages/sage-assets/lib/stylesheets/components/_loader.scss(1 hunks)packages/sage-assets/lib/stylesheets/components/_media_tile.scss(2 hunks)packages/sage-assets/lib/stylesheets/components/_menu_button.scss(2 hunks)packages/sage-assets/lib/stylesheets/components/_meter.scss(1 hunks)packages/sage-assets/lib/stylesheets/components/_modal.scss(6 hunks)packages/sage-assets/lib/stylesheets/components/_nav.scss(5 hunks)packages/sage-assets/lib/stylesheets/components/_overlay.scss(1 hunks)packages/sage-assets/lib/stylesheets/components/_page_heading.scss(2 hunks)packages/sage-assets/lib/stylesheets/components/_pagination.scss(3 hunks)packages/sage-assets/lib/stylesheets/components/_panel.scss(3 hunks)packages/sage-assets/lib/stylesheets/components/_panel_controls.scss(1 hunks)packages/sage-assets/lib/stylesheets/components/_panel_figure.scss(1 hunks)packages/sage-assets/lib/stylesheets/components/_popover.scss(1 hunks)packages/sage-assets/lib/stylesheets/components/_progress_bar.scss(1 hunks)packages/sage-assets/lib/stylesheets/components/_property.scss(1 hunks)packages/sage-assets/lib/stylesheets/components/_radio.scss(2 hunks)packages/sage-assets/lib/stylesheets/components/_search.scss(4 hunks)packages/sage-assets/lib/stylesheets/components/_sidebar.scss(1 hunks)packages/sage-assets/lib/stylesheets/components/_stat_box.scss(3 hunks)packages/sage-assets/lib/stylesheets/components/_status_icon.scss(2 hunks)packages/sage-assets/lib/stylesheets/components/_switch.scss(4 hunks)packages/sage-assets/lib/stylesheets/components/_tab.scss(2 hunks)packages/sage-assets/lib/stylesheets/components/_table.scss(2 hunks)packages/sage-assets/lib/stylesheets/components/_tag.scss(4 hunks)packages/sage-assets/lib/stylesheets/components/_toast.scss(7 hunks)packages/sage-assets/lib/stylesheets/components/_tooltip.scss(2 hunks)packages/sage-assets/lib/stylesheets/components/_topbar.scss(1 hunks)packages/sage-assets/lib/stylesheets/components/_transaction_card.scss(3 hunks)packages/sage-assets/lib/stylesheets/components/_typeahead.scss(4 hunks)packages/sage-assets/lib/stylesheets/components/_upload_card.scss(4 hunks)packages/sage-assets/lib/stylesheets/core/_typography.scss(5 hunks)packages/sage-assets/lib/stylesheets/core/_variables.scss(3 hunks)packages/sage-assets/lib/stylesheets/core/mixins/_sage.scss(9 hunks)packages/sage-assets/lib/stylesheets/global/_reboot.scss(2 hunks)packages/sage-assets/lib/stylesheets/layout/_content.scss(1 hunks)packages/sage-assets/lib/stylesheets/layout/_frame.scss(1 hunks)packages/sage-assets/lib/stylesheets/tokens/_border.scss(1 hunks)packages/sage-assets/lib/stylesheets/tokens/_color_palette.scss(1 hunks)packages/sage-assets/lib/stylesheets/tokens/_ds_tokens_dark_mode.scss(1 hunks)packages/sage-assets/lib/stylesheets/tokens/_shadow.scss(1 hunks)packages/sage-assets/lib/stylesheets/tokens/_type_specs.scss(1 hunks)packages/sage-assets/lib/stylesheets/vendor/_parsley.scss(1 hunks)packages/sage-assets/package.json(1 hunks)packages/sage-packs/CHANGELOG.md(1 hunks)packages/sage-packs/package.json(2 hunks)packages/sage-react/CHANGELOG.md(1 hunks)packages/sage-react/package.json(2 hunks)
💤 Files with no reviewable changes (2)
- .github/workflows/codeql.yml
- .github/workflows/dependency-review.yml
🧰 Additional context used
🪛 GitHub Actions: Publish
docs/lib/sage-frontend/stylesheets/docs/_colors.scss
[error] 71-71: SassError: $color: sage-color-hex(blue, 100) is not a color.
packages/sage-assets/lib/stylesheets/components/_typeahead.scss
Outdated
Show resolved
Hide resolved
packages/sage-assets/lib/stylesheets/tokens/_ds_tokens_dark_mode.scss
Outdated
Show resolved
Hide resolved
pixelflips
left a comment
pixelflips
left a comment
There was a problem hiding this comment.
Couple of small ones and check the coderabbit comments
packages/sage-assets/lib/stylesheets/tokens/_ds_tokens_dark_mode.scss
Outdated
Show resolved
Hide resolved
|
@coderabbitai review this PR |
|
@QuintonJason, I'll review this PR for you. This appears to be a development version bump (6.2.25-dev.19) with significant theming changes including:
I'll analyze the changes and provide detailed feedback. ✅ Actions performedReview triggered.
|
|
@coderabbitai review this pr |
|
@QuintonJason, I'll review this PR for you. ✅ Actions performedReview triggered.
|
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (2)
packages/sage-assets/lib/stylesheets/components/_button.scss (1)
323-323: Remove redundant background-color declaration.Line 323 is immediately overridden by line 324. This redundancy was flagged in previous reviews.
Apply this diff to remove the duplicate:
height: rem(38px); - background-color: var(--pine-color-background-container); background-color: var(--pine-color-secondary); border-color: transparent;packages/sage-assets/lib/stylesheets/components/_dropdown.scss (1)
341-344: Remove duplicate dark mode override.Lines 341-344 duplicate the dark theme styles already defined at lines 315-318. This redundancy was flagged in previous reviews.
Apply this diff to remove the duplicate:
.sage-search ~ & { border-start-start-radius: 0; border-start-end-radius: 0; } - - [data-theme="dark"] & { - background: var(--pine-color-background-container); - border: var(--pine-border); - }
🧹 Nitpick comments (1)
packages/sage-assets/lib/stylesheets/components/_link.scss (1)
46-58: Use semantic tokens for consistency in danger link variant hover state.The danger variant mixes semantic tokens (
--pine-color-danger) with raw color tokens (--pine-color-red-950at line 51,--pine-color-red-300at line 55). For consistency with other link variants (primary, secondary, neutral) and the broader design system, use the semantic token--pine-color-danger-hoverfor the hover state instead of the raw--pine-color-red-950.
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (9)
docs/app/views/application/_app_head_content.html.erb(1 hunks)packages/sage-assets/lib/stylesheets/components/_button.scss(7 hunks)packages/sage-assets/lib/stylesheets/components/_dropdown.scss(11 hunks)packages/sage-assets/lib/stylesheets/components/_hint.scss(1 hunks)packages/sage-assets/lib/stylesheets/components/_input_group.scss(3 hunks)packages/sage-assets/lib/stylesheets/components/_link.scss(6 hunks)packages/sage-assets/lib/stylesheets/components/_property.scss(1 hunks)packages/sage-assets/lib/stylesheets/components/_toast.scss(7 hunks)packages/sage-assets/lib/stylesheets/components/_typeahead.scss(4 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- packages/sage-assets/lib/stylesheets/components/_input_group.scss
- docs/app/views/application/_app_head_content.html.erb
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: publish
🔇 Additional comments (17)
packages/sage-assets/lib/stylesheets/components/_typeahead.scss (2)
63-63: Previous concern addressed—focus ring now uses semantic token.The focus outline now correctly uses
var(--pine-color-focus-ring)instead of the hardcoded purple color, aligning with badge, button, and other components for consistency.
27-27: Color token migrations are semantically appropriate.The CSS custom property substitutions correctly map to their semantic contexts:
--pine-color-background-containerfor panel background--pine-color-text-tertiaryfor de-emphasized null state text--pine-color-background-mutedfor subtle hover/focus backgrounds--pine-color-text-strongfor emphasized hover textAlso applies to: 35-35, 52-52, 104-104
packages/sage-assets/lib/stylesheets/components/_hint.scss (1)
8-8: Text color token migration looks good.The migration from
sage-color(grey, 900)tovar(--pine-color-text)is semantically appropriate for text content.packages/sage-assets/lib/stylesheets/components/_link.scss (5)
7-10: LGTM: Clean variable definitions with semantic tokens.The variable definitions appropriately use semantic Pine CSS custom properties for link colors and states.
12-34: LGTM: Consistent semantic token usage for primary and secondary variants.The primary and secondary link variants correctly use text-based semantic tokens for all states.
35-45: Previous issue resolved: Now using semantically correct text tokens.The neutral variant now correctly uses
text-inverse-emphasisandtext-inversetokens instead of background tokens. This is semantically appropriate for inverse text styling (light text on dark backgrounds).
72-81: LGTM: Appropriate semantic token usage.The hover, focus, and help link color updates correctly use semantic Pine tokens. Line 227 uses a specific color token (
purple-600) for the focus outline, which appears intentional for consistent focus styling across the design system.Also applies to: 182-182, 196-196, 200-200, 227-227
166-170: Verify contrast for dark mode override.The dark theme override sets
.sage-alert .sage-link--secondarytogrey-900, which is a very dark color. Confirm that this provides adequate contrast (WCAG AA minimum 4.5:1 for normal text) against the.sage-alertbackground in dark mode.packages/sage-assets/lib/stylesheets/components/_property.scss (4)
15-15: Past review comment addressed.The migration to
var(--pine-dimension-md)has been completed as requested in the previous review.
20-20: LGTM!Spacing migration to
var(--pine-dimension-xs)is consistent with the Pine CSS custom properties pattern.
27-29: No action required on.sage-property__valuestyling.The file was added as new, not migrated. The
.sage-property__valueselector has always contained only@include truncate(). The parent.sage-propertyusesfont: var(--pine-typography-body-medium), which provides the typography styling for the entire component. No font-weight was removed; this is the intended design.Likely an incorrect or invalid review comment.
11-12: Verify typography size change from "small" to "medium" is intentional.The migration replaced the Sage typography placeholder with
var(--pine-typography-body-medium). Font size mappings show this increases the size from 12px (small) to 14px (medium). Other components in the codebase still use the body-small size, suggesting this may be an intentional design change for the property component. Confirm with the design team that this size increase is correct for this component.packages/sage-assets/lib/stylesheets/components/_toast.scss (5)
73-76: Color token migration looks correct.The migration from Sage color functions to Pine CSS custom properties is properly implemented for the base toast styles. White text on grey-900 background maintains good contrast.
97-97: Danger variant migration is correct.The danger background color correctly uses the Pine CSS custom property.
109-142: Text color migrations are consistent.All text elements (icon, value, button states) correctly use the white color token, maintaining readability across the dark toast backgrounds.
151-151: Hover state color migration approved.The grey-150 color token maintains excellent contrast ratios on both danger and grey backgrounds (4.24:1 and 11.06:1 respectively), meeting WCAG AA standards.
164-164: Verify focus ring color meets WCAG AA contrast requirements against actual token value.The focus outline now uses
var(--pine-color-focus-ring)from the Pine Design System instead of the previous purple-600. While this token is used consistently across 16+ components and introduced in a recent token update, the actual color value of this external token cannot be verified within the sage-lib codebase. The focus indicator must maintain at least 3:1 contrast ratio against toast backgrounds (danger red, grey-900 #343332) to meet WCAG AA standards. Confirm the token resolves to an accessible color by checking the Pine DS library documentation or testing the component in a browser with the actual token values.
3 participants
Uh oh!
There was an error while loading. Please reload this page.