Add colors in JS and Color Palette #18

snedadah · 2022-06-18T20:40:27-04:00

snedadah commented

2022-06-18 20:40:27 -04:00

After further investgation, the solutions that we planned on using (getComputedStyle(document.documentElement) .getPropertyValue(property);) did not end up working, because of some weird issues with nextjs. However, luckly it seems we can still just use the css variables inside the js (I tried it out with Amy's bar graph demo and it seemed to work well).

Thus I created a typesafe helper, utils/Colors.ts , anytime you need a color, you can use this object, eg.

import { Colors } from "utils/Colors";
<Text fill={Colors.primaryBackground}> ... </Text>

And importantly the Colors is typesafe so you can get autocomplete. (Protip: press Cntrl + Space if you want autocomplete to pop up if it is not coming)

Staging: https://shahanneda-js-colors-csc-class-profile-staging-snedadah.k8s.csclub.cloud

After further investgation, the solutions that we planned on using (`getComputedStyle(document.documentElement) .getPropertyValue(property);`) did not end up working, because of some weird issues with nextjs. However, luckly it seems we can still just use the css variables inside the js (I tried it out with Amy's bar graph demo and it seemed to work well). Thus I created a typesafe helper, utils/Colors.ts , anytime you need a color, you can use this object, eg. import { Colors } from "utils/Colors"; <Text fill={Colors.primaryBackground}> ... </Text> And importantly the Colors is typesafe so you can get autocomplete. (Protip: press Cntrl + Space if you want autocomplete to pop up if it is not coming) Staging: https://shahanneda-js-colors-csc-class-profile-staging-snedadah.k8s.csclub.cloud

snedadah added 2 commits 2022-06-18 20:40:28 -04:00

47cfd56471

Added color js helper and color palette

08dfe42b68

Removed unused comment

snedadah requested review from j285he 2022-06-18 20:41:30 -04:00

snedadah requested review from a258wang 2022-06-18 20:41:30 -04:00

snedadah requested review from b72zhou 2022-06-18 20:41:30 -04:00

snedadah requested review from e26chiu 2022-06-18 20:41:39 -04:00

snedadah changed title from ~~Add colors in JS and Color Palate~~ to Add colors in JS and Color Palette

2022-06-18 20:43:57 -04:00

a258wang reviewed 2022-06-28 23:11:24 -04:00

components/ColorPalette.module.css Outdated

													
				@ -0,0 +1,20 @@

				.box {

					width: 100px;

					height: 100px;

a258wang commented

2022-06-28 23:11:24 -04:00

Two nits:

The indents here look a bit wide, could we make sure that we're using 2 spaces for indents? (We should probably turn on format-on-save for CSS lol)
Not a big deal since this is just a demo component, but let's use the calc(100rem / 16) syntax that we're accustomed to from the website. I think the CSS plugins here should be configured so that such calc statements get computed as 6.25rem in the exported site, similar to what we have for the website.

Two nits: 1. The indents here look a bit wide, could we make sure that we're using 2 spaces for indents? (We should probably turn on format-on-save for CSS lol) 2. Not a big deal since this is just a demo component, but let's use the `calc(100rem / 16)` syntax that we're accustomed to from the website. I think the CSS plugins here should be configured so that such `calc` statements get computed as `6.25rem` in the exported site, similar to what we have for the website.

snedadah marked this conversation as resolved

a258wang reviewed 2022-06-28 23:16:50 -04:00

utils/Colors.ts Outdated

													
				@ -0,0 +20,4 @@

				type ColorName = typeof colorNames[number];

				export const Colors: { [key in ColorName]: string } = {

				  primaryBackground: "var(--primary-background)",

a258wang commented

2022-06-28 23:16:50 -04:00

I think there's probably a way to directly map the colorNames array to an object of this format, so that in the future if we add more colours we only need to update the one array... but since this is just a demo component I won't be too picky about it. 😆

Also, a super nit: consider naming the object Color (singular) so that individual colours can be accessed like Color.primaryBackground instead of Colors.primaryBackground - makes more sense IMO since we're just using one colour at a time.

I think there's probably a way to directly map the `colorNames` array to an object of this format, so that in the future if we add more colours we only need to update the one array... but since this is just a demo component I won't be too picky about it. 😆 Also, a super nit: consider naming the object `Color` (singular) so that individual colours can be accessed like `Color.primaryBackground` instead of `Colors.primaryBackground` - makes more sense IMO since we're just using one colour at a time.

snedadah commented

2022-06-29 00:02:39 -04:00

True, I think Color is used more often in order codebases as well.

snedadah marked this conversation as resolved

snedadah added 1 commit 2022-06-28 23:56:47 -04:00

902eb989dc

Updated code style

snedadah added 1 commit 2022-06-29 00:02:03 -04:00