Add colors in JS and Color Palette #18

Merged
snedadah merged 5 commits from shahanneda/js-colors into main 3 months ago
Collaborator

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 4 months ago
snedadah requested review from j285he 4 months ago
snedadah requested review from a258wang 4 months ago
snedadah requested review from b72zhou 4 months ago
snedadah requested review from e26chiu 4 months ago
snedadah changed title from Add colors in JS and Color Palate to Add colors in JS and Color Palette 4 months ago
a258wang reviewed 3 months ago
.box {
width: 100px;
height: 100px;
Owner

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.
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 3 months ago
utils/Colors.ts Outdated
type ColorName = typeof colorNames[number];
export const Colors: { [key in ColorName]: string } = {
primaryBackground: "var(--primary-background)",
Owner

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.
Poster
Collaborator

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

True, I think Color is used more often in order codebases as well.
snedadah marked this conversation as resolved
snedadah added 1 commit 3 months ago
snedadah added 1 commit 3 months ago
snedadah added 1 commit 3 months ago
408527e762
Updated margin
a258wang approved these changes 3 months ago
snedadah merged commit 126a61fc28 into main 3 months ago
snedadah deleted branch shahanneda/js-colors 3 months ago

Reviewers

j285he was requested for review 4 months ago
b72zhou was requested for review 4 months ago
e26chiu was requested for review 4 months ago
a258wang approved these changes 3 months ago
continuous-integration/drone/push Build is passing
The pull request has been merged as 126a61fc28.
Sign in to join this conversation.
No Milestone
No project
No Assignees
2 Participants
Notifications
Due Date

No due date set.

Dependencies

No dependencies set.

Reference: www/cs-2022-class-profile#18
Loading…
There is no content yet.