Skip to content

ASAB Shell - Styling - Define secondary text color in dark mode#59

Open
JanMajer86 wants to merge 8 commits intomainfrom
feature/secondary-color-styling-dark-mode
Open

ASAB Shell - Styling - Define secondary text color in dark mode#59
JanMajer86 wants to merge 8 commits intomainfrom
feature/secondary-color-styling-dark-mode

Conversation

@JanMajer86
Copy link
Copy Markdown
Collaborator

@JanMajer86 JanMajer86 commented Mar 13, 2026

  • fill in missing rgb definition
  • after messing with theme styling and breaking everything, i reverted styling back and tried this approach:
    <div className='bg-body-secondary text-body-secondary>
    which gave me this result:
Screenshot from 2026-03-25 16-36-37 Screenshot from 2026-03-25 16-36-28

@JanMajer86 JanMajer86 self-assigned this Mar 13, 2026
@JanMajer86 JanMajer86 marked this pull request as draft March 13, 2026 15:36
@coderabbitai
Copy link
Copy Markdown

coderabbitai bot commented Mar 13, 2026

📝 Walkthrough

Walkthrough

Adds an RGB variant for nightfall blue and introduces Bootstrap text color variables (primary and secondary, with RGB counterparts) in both light and dark theme files; fixes several RGB references in the dark theme to use the correct -rgb variables.

Changes

Cohort / File(s) Summary
Color Constants
src/styles/constants/colors.scss
Added --nightfall-blue-rgb: 25, 34, 63; to :root alongside existing --nightfall-blue.
Theme — Dark
src/styles/constants/theme-dark.scss
Added --bs-text-primary/--bs-text-primary-rgb and --bs-text-secondary/--bs-text-secondary-rgb; corrected --bs-link-hover-color-rgb reference and changed --bs-input-bg-color-rgb / --bs-element-bg-color-rgb to use -rgb variants (including --nightfall-blue-rgb).
Theme — Light
src/styles/constants/theme-light.scss
Added --bs-text-primary/--bs-text-primary-rgb and --bs-text-secondary/--bs-text-secondary-rgb mapped to appropriate color variables under :root, [data-bs-theme=light].

Estimated code review effort

🎯 2 (Simple) | ⏱️ ~10 minutes

Suggested reviewers

  • Pe5h4
  • ateska

Poem

🐰
I hopped through stylesheets late at night,
Adding a pinch of RGB light,
Nightfall blue in numbers true,
Themes now whisper, dark and bright,
A tiny rabbit's CSS delight. 🎨

🚥 Pre-merge checks | ✅ 3
✅ Passed checks (3 passed)
Check name Status Explanation
Title check ✅ Passed The title focuses on defining secondary text color in dark mode, which is only one aspect of the changeset. The PR also adds secondary color definitions for light mode and corrects missing RGB definitions, making the title incomplete.
Docstring Coverage ✅ Passed No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.

✨ Finishing Touches
📝 Generate docstrings
  • Create stacked PR
  • Commit on current branch
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch feature/secondary-color-styling-dark-mode

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.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

@JanMajer86 JanMajer86 requested a review from Pe5h4 March 16, 2026 10:00
@JanMajer86 JanMajer86 marked this pull request as ready for review March 16, 2026 13:48
Copy link
Copy Markdown

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 1

🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.

Inline comments:
In `@src/styles/constants/theme-dark.scss`:
- Around line 14-15: Delete the non-standard CSS variables --bs-text-secondary
and --bs-text-secondary-rgb from the theme files: remove their declarations in
src/styles/constants/theme-dark.scss and src/styles/constants/theme-light.scss
(they duplicate --bs-secondary / --bs-secondary-rgb and are not referenced
elsewhere); ensure only the official Bootstrap variables --bs-*-color and
--bs-*-color-rgb remain per the file header.

ℹ️ Review info
⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro

Run ID: 3f025ca0-458a-4b50-b311-d9bb46bea11e

📥 Commits

Reviewing files that changed from the base of the PR and between d8ef8bf and 90bea04.

📒 Files selected for processing (3)
  • src/styles/constants/colors.scss
  • src/styles/constants/theme-dark.scss
  • src/styles/constants/theme-light.scss

@@ -39,7 +42,7 @@
--bs-input-bg-color-rgb: var(--midnight-blue);
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

here is missing the rgb part as well

@Pe5h4
Copy link
Copy Markdown
Collaborator

Pe5h4 commented Mar 19, 2026

@JanMajer86 Ok, as per discussion under the ticket, I am reverting what I wrote. In that case it seems ok. I would also expect to have --bs-text-primary as well

@Pe5h4 Pe5h4 self-requested a review March 19, 2026 08:26
--bs-secondary: var(--lavender-mist);
--bs-secondary-rgb: var(--lavender-mist-rgb);

--bs-text-primary: var(--black);
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should this color be really black in light mode?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants