Fix Issue #4554 - Replace hardcoded colors with Theme variables in Notifications#4573
Conversation
- Add stripMarkdownAndHtml utility function to clean formatting - Apply cleaning to news summary display in NewsListItem - Add comprehensive unit tests for the utility function - Fixes issue where raw Markdown/HTML tags were visible in summaries
…Issue ONEARMY#4554) - Replace activeYellow with theme.colors.primary in NotificationListSupabase filter buttons - Replace #fff0b4 and activeYellow with theme colors in NotificationItemSupabase - Use 40% opacity of primary color for unread notification backgrounds - Colors now dynamically change based on theme (PP: yellow, PK: green, FF: red)
mariojsnunes
left a comment
There was a problem hiding this comment.
Nice!
I see some changes from the other PR in here, should keep it separate.
| /** | ||
| * Converts a hex color to rgba with specified opacity | ||
| */ | ||
| const hexToRgba = (hex: string, opacity: number): string => { |
There was a problem hiding this comment.
I think this is fine, but I'm wondering if it would be better to have a "faded" color setting on the theme.
@dalibormrska, @jproberson any thoughts on that?
|
So I think there's a much simplier approach for this. Rather than new functions or calling Remove |
agree. maybe |
|
Hey @KostasKoukianakis , Thanks for jumping in and wanting to help out, we really do appreciate it. That said, we're going to close these PRs for now. I wanted to explain why so it doesn't feel like we're just shutting you down. The main issue is that each PR has commits from your other branches mixed in. When we try to review one feature, we're seeing changes from unrelated work, which makes it hard to review anything in isolation. We're a small team, and we just don't have the bandwidth to untangle overlapping changes across 6-7 PRs at once. We'd much rather work with you on one thing at a time, give you real feedback, and help you get integrated in our ecosystem. If you want to keep contributing (and we hope you do), here's what we'd suggest: start fresh from master with one issue, keep that PR focused on just that work, and let's get it reviewed and merged before moving to the next one. We're actively updating our contributing docs to make this clearer for everyone going forward. Hope that all makes sense. Happy to chat if you have questions. |
PR Checklist
PR Type
What kind of change does this PR introduce?
What is the new behavior?
Before: Notification elements (filter buttons, unread notification borders, and backgrounds) used hardcoded Precious Plastic yellow colors (
activeYellowand#fff0b4) regardless of the active theme.After: Notification colors now dynamically adapt to the active theme:
#fee77b)#8ab57f)#f82f03)Changes Made:
theme.colors.primaryinstead of hardcodedactiveYellowtheme.colors.primaryand backgrounds use 40% opacity of the primary color (replacing hardcoded#fff0b4andactiveYellow)Visual Changes:
Does this PR introduce a DB Schema Change or Migration?
Git Issues
Closes #4554
What happens next?
Thank you for the contribution! We will review it ASAP.
If you need more immediate feedback you can reach out to us on Discord in the Community Platform
developmentchannel.