Details
Visual inspection:
Diff Detail
- Repository
- rCOMM Comm
- Lint
No Lint Coverage - Unit
No Test Coverage
Event Timeline
Looks good. Ideally we'd define those colors in colors.js and use them here instead of directly using hex values.
| native/account/logged-out-modal.react.js | ||
|---|---|---|
| 652 ↗ | (On Diff #19787) | Ideally we'd assign a name to this hex value in colors.js and consume that here. |
| native/account/logged-out-modal.react.js | ||
|---|---|---|
| 652 ↗ | (On Diff #19787) | So historically, colors.js was just about light/dark mode. We never used it in LoggedOutModal since its design is the same in both. But it sounds like now colors.js is also serving the purpose of collecting all of the colors in the app in one place, and trying to enforce somewhat of a "design system" with fewer one-off colors. I can add this there with the same color for light and dark, but given that doesn't usually happen in colors.js this will almost definitely need to be a new color name. |
| native/account/logged-out-modal.react.js | ||
|---|---|---|
| 652 ↗ | (On Diff #19787) |
Yeah pretty much... or at least that's the feedback I got at some point (I think Ben was advocating for colors.js on native to used similarly to theme.css on web) and have been giving to others since. |