-
Notifications
You must be signed in to change notification settings - Fork 17
Improve UI phase2 #419
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Improve UI phase2 #419
Conversation
- Remove red2 (duplicate of statusError) - Replace sellColor with red1 across 5 files
- Remove purpleAccent (#764BA2) - Replace with purpleButton (#7856AF) across 6 files - Consolidate 10 instances to unified purple color
WalkthroughThis PR consolidates color constants across the design system by removing deprecated colors (red2, sellColor, purpleAccent) and introducing standardized replacements (red1, purpleButton, statusError). All usages throughout the codebase are updated to reference the new consolidated colors, and design system documentation is updated to reflect completion of Phases 1, 2, and 2b of the color consolidation effort. Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~15 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 inconclusive)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. 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 |
There was a problem hiding this 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
🤖 Fix all issues with AI agents
In `@docs/architecture/DESIGN_SYSTEM.md`:
- Around line 351-354: Update the Goals section text to remove the contradictory
"deferred" phrasing by explicitly stating that purple consolidation was moved to
Phase 2b and is now completed; locate the "Goals:" block (the bulleted items
including "Consolidate 3 red variants → 2 red variants" and the purple
consolidation line) and replace the "⏸️ Purple consolidation deferred (out of
scope for Phase 2)" line with wording such as "✔️ Purple consolidation moved to
Phase 2b and completed" so the document no longer contradicts the Phase 2b
completion status.
| **Goals:** | ||
| 1. Consolidate 3 red variants → 2 red variants (or justify keeping 3) | ||
| 2. Clarify purple usage or consolidate to 1 purple | ||
| 3. Document clear usage guidelines for each color | ||
|
|
||
| #### Work Required | ||
| 1. ✅ Consolidate 3 red variants → 2 red variants with clear semantics | ||
| 2. ⏸️ Purple consolidation deferred (out of scope for Phase 2) | ||
|
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Clarify Phase 2 vs Phase 2b wording to avoid contradiction.
Line 353 says purple consolidation was deferred, but later the doc marks Phase 2b as complete. Consider updating this line to reflect that purple moved to Phase 2b and is now done.
✏️ Suggested wording tweak
-2. ⏸️ Purple consolidation deferred (out of scope for Phase 2)
+2. ✅ Purple consolidation completed in Phase 2b (see Phase 2b section)📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| **Goals:** | |
| 1. Consolidate 3 red variants → 2 red variants (or justify keeping 3) | |
| 2. Clarify purple usage or consolidate to 1 purple | |
| 3. Document clear usage guidelines for each color | |
| #### Work Required | |
| 1. ✅ Consolidate 3 red variants → 2 red variants with clear semantics | |
| 2. ⏸️ Purple consolidation deferred (out of scope for Phase 2) | |
| **Goals:** | |
| 1. ✅ Consolidate 3 red variants → 2 red variants with clear semantics | |
| 2. ✅ Purple consolidation completed in Phase 2b (see Phase 2b section) |
🤖 Prompt for AI Agents
In `@docs/architecture/DESIGN_SYSTEM.md` around lines 351 - 354, Update the Goals
section text to remove the contradictory "deferred" phrasing by explicitly
stating that purple consolidation was moved to Phase 2b and is now completed;
locate the "Goals:" block (the bulleted items including "Consolidate 3 red
variants → 2 red variants" and the purple consolidation line) and replace the
"⏸️ Purple consolidation deferred (out of scope for Phase 2)" line with wording
such as "✔️ Purple consolidation moved to Phase 2b and completed" so the
document no longer contradicts the Phase 2b completion status.
grunch
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Le pedí a una IA que investigue si es buena idea usar texto negro sobre rojo, aquí la respuesta:
1. ¿Texto negro sobre botón rojo es accesible?
Depende del rojo concreto, pero en general es una combinación riesgosa.
- Con rojo puro brillante (#FF0000) y texto negro, el contraste teórico ronda ~5.25:1, lo que cumple WCAG AA para texto normal.
- El problema real: casi ningún botón usa rojo puro.
En la práctica se usan rojos oscuros, desaturados o con gradiente, y ahí el contraste cae por debajo de 4.5:1, fallando accesibilidad.
👉 Conclusión técnica:
Negro sobre rojo puede cumplir en laboratorio, pero falla fácilmente en producción.
2. Impacto específico en personas mayores 👵👴
Sí, puede afectar claramente la lectura, por varios motivos bien documentados:
Cambios visuales con la edad
- Disminuye la sensibilidad al contraste
- El cristalino se vuelve más amarillento, lo que reduce la percepción del rojo
- Más dificultad para distinguir bordes oscuros sobre fondos saturados
Resultado:
El texto negro “se hunde” visualmente en el rojo, sobre todo en pantallas pequeñas y con brillo bajo.
Esto no es solo teoría: en tests de usabilidad, personas mayores tardan más en leer botones rojo+negro que rojo+blanco o rojo+blanco con ícono.
3. ¿Es buena práctica poner texto negro sobre rojo?
📚 Estado de la práctica (UX / accesibilidad)
No es una buena práctica recomendada.
De hecho:
- Material Design (Google) → rojo con texto blanco
- Apple HIG → botones destructivos rojo + blanco
- Gov.uk Design System → rojo oscuro + texto blanco, alto contraste
- WCAG advisory techniques → evitar texto oscuro sobre colores saturados
No porque “esté prohibido”, sino porque:
- es frágil al contraste
- falla con visión envejecida
- falla con brillo bajo / modo exterior
- falla con pantallas OLED mal calibradas
4. Problema extra: semántica + legibilidad
El botón rojo suele significar:
- peligro
- acción destructiva
- alto riesgo
Si además:
- el texto es negro
- el peso tipográfico no es fuerte
➡️ el usuario percibe urgencia, pero no entiende rápido qué hace
Eso es peligroso en flujos financieros.
5. Recomendaciones concretas (prácticas y seguras)
✅ Mejor opción
- Texto blanco (#FFFFFF) sobre rojo
- Contraste ≥ 4.5:1 (ideal ≥ 7:1)
- Fuente semibold o bold
- Tamaño mínimo 14–16 px en mobile
✅ Aún mejor
- Blanco + ícono (↑ vender,
⚠️ riesgo) - Bordes claros o sombra suave para separar del fondo
❌ Evitar
- Negro sobre rojo oscuro
- Negro sobre rojo con gradiente
- Tipografía fina
- Texto pequeño
|
Por lo que menciona @grunch fue que le baje la opacidad a ese boton, capaz no es necesario tener un solo rojo sino distintos todos segun el uso, lo mismo con los verdes y otros colores y estandarizar mas sus usos |

conslidate purple and red color
Summary by CodeRabbit
Style
Documentation
✏️ Tip: You can customize this high-level summary in your review settings.