Skip to content

refactor(css): replace hardcoded colors and undefined var with theme variables#4656

Open
lucasgab2230 wants to merge 2 commits into
odysseus-dev:devfrom
lucasgab2230:dev
Open

refactor(css): replace hardcoded colors and undefined var with theme variables#4656
lucasgab2230 wants to merge 2 commits into
odysseus-dev:devfrom
lucasgab2230:dev

Conversation

@lucasgab2230

Copy link
Copy Markdown

Summary

  • Fix var(--danger, #f44) → var(--color-error) (--danger is undefined), remove redundant hex fallbacks from var(--color-muted), replace hardcoded #d19a66 with var(--accent-warm) in note/orange and classes and memory-cat-task, matching the existing :root variable

Target branch

  • This PR targets dev, not main. All PRs land in dev; main is curated by the maintainer at each release. If your PR is on main by accident, click "Edit" on this PR and change the base.

Linked Issue

Fixes #4655

Type of Change

  • Bug fix (non-breaking — fixes a confirmed issue)
  • New feature (non-breaking — adds new behaviour)
  • Breaking change (changes or removes existing behaviour)
  • Refactor / cleanup (behaviour unchanged)
  • Documentation only
  • CI / tooling / configuration

Checklist

  • I searched open issues and open PRs — this is not a duplicate.
  • This PR targets dev
  • My changes are limited to the scope described above — no unrelated refactors or whitespace changes mixed in.
  • I actually ran the app (docker compose up or uvicorn app:app) and verified the change works end-to-end. Type-checks and unit tests are not enough.

How to Test

  1. git clone https://github.com/pewdiepie-archdaemon/odysseus.git
  2. cd odysseus
  3. cp .env.example .env
  4. docker compose up -d --build
  5. Open http://localhost:7000 when the containers are healthy. The first admin password is printed in docker compose logs odysseus.

Visual / UI changes — REQUIRED if you touched anything that renders

Anything that changes what the UI looks like — buttons, icons, padding, colors, fonts, spacing, layout, CSS, HTML, SVG, or any static/js/ module that draws to the DOM — needs all of the following. PRs that change rendering without these WILL be closed.

  • Screenshot or short clip of the change in the running app, attached below. Mobile screenshot too if the change affects mobile.
  • Style match: the change uses Odysseus's existing visual language. Specifically:
    • Reuse existing CSS variables (--red, --fg, --bg, --card, --border, etc.) — do not introduce new color values, font sizes, or spacing units.
    • Reuse existing button/input/card/border classes. Don't invent parallel styling.
    • No Unicode emoji in UI or code. Use inline SVG (matching the monochrome icon style already in static/index.html) or plain text.
    • Monospaced font (Fira Code) for primary UI text. Don't override.
    • Dark theme is the default; any light-mode work must be wired through the existing theme system, not hard-coded.
  • No new component patterns. If a similar widget already exists in the app, extend it instead of writing a parallel one.
  • [] I am not an LLM agent submitting a bulk PR. If you are, please open an issue describing the problem first — bulk auto-generated PRs that don't match the project's visual style are closed on sight, even when the underlying fix is correct.

Screenshots / clips

@github-actions github-actions Bot added the ready for review Description complete — ready for maintainer review label Jun 20, 2026

@vdmkenny vdmkenny left a comment

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.

Nice cleanup, this is exactly the direction the style guide wants. I verified the values all line up so there's no visual change in the default theme:

  • --danger is genuinely undefined, so the old color was the #f44 fallback; --color-error is #ff4444 (the same color), now semantic and theme-aware.
  • --color-muted is defined as #888, so the #888 fallback was redundant and safe to drop.
  • --accent-warm is #d19a66, an exact match for the hardcoded values you replaced, and you correctly left the separate --hl-number (same hex, different meaning) alone.

One thing before it can merge: CONTRIBUTING.md asks for a screenshot on any CSS change ("if you are unsure whether a change is visual, it is"), so please run it locally and attach a quick capture of the affected elements (an orange note, error text, and a muted/context label) so it's on record that they render unchanged. Since this is a value-equivalent refactor that screenshot doubles as the regression check. Looks good otherwise.

@lucasgab2230

Copy link
Copy Markdown
Author

Ok, wait a moment.

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

Labels

ready for review Description complete — ready for maintainer review

Projects

None yet

Development

Successfully merging this pull request may close these issues.

The static/styles.css has some issues on the code. I'll open a PR refactoring this file.

2 participants