-
Notifications
You must be signed in to change notification settings - Fork 0
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
Change color palette to green #102
Conversation
🦙 MegaLinter status: ✅ SUCCESS
See detailed report in MegaLinter reports |
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.
This is working well and looking nice. From browsing through it looks like the only component (beside AnimatedArcAndMap.tsx
) that is still using it's own hardcoded colors is AnimatedMapLegend.tsx
that doesn't look like it's being used by any other component and could be removed I think (unless you have plans for it).
The only other thing I noted was a note about color/key naming in the theming, but that decision may need to be between the devs and UX, but otherwise this is good to go.
@@ -38,7 +38,7 @@ export default function useCreateArcPath( | |||
arcPathsReference.current[state] = new L.Curve( | |||
['M', DC_CENTER, 'Q', midpoint, polygonCenterTuple], | |||
{ | |||
color: '#2051FF', | |||
color: '#85bb65', // the actual shade of US legal tender :-O |
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.
I know this is a bit of an arbitrary choice but this does make a good color for this palette.
Also more emoticons in comments please
green: { | ||
50: '#F1F8F3', | ||
100: '#DEECE2', | ||
300: '#ADCDB7', | ||
600: '#3DA45A', | ||
900: '#235F34', | ||
}, |
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.
Centralizing the colors like this is an excellent choice and will absolutely make things easier to keep track of.
However, while I understand the use of the number keys coordinating with the numbers in the FIGMA design; Using semantic names may help devs more easily remember what is available and which colors are which (e.g. 'light', 'medium', 'dark')
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.
Interesting! I'm only parroting what I'm used to from azavea/iow-boundary-tool. I can imagine with six or seven shades that 'medium' becomes less helpful (extra medium?) I'm wondering if you contemplated that on any other projects?
Chakra also guides toward numbers: https://chakra-ui.com/docs/styled-system/theme
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.
I thought we made that choice in a recent Civic Apps project, but can't find the example I was looking for, but also for that we had 5 distinct colors with a light and dark variant, so that may not apply here since this is a monochromatic gradient.
Good spot, that legend was completely obviated by f38746d, so I'll remove it. |
Unused since f38746d.
e6dd97e
to
9fa9aaa
Compare
Overview
Change to green color scheme.
Closes #93
Demo
Notes
Testing Instructions
I also went ahead and deployed this so that the team could more easily have a look.
Checklist
fixup!
commits have been squashedCHANGELOG.md
updated with summary of features or fixes, following Keep a Changelog guidelinesREADME.md
updated if necessary to reflect the changes