Skip to content
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

Dev -> Main #634

Merged
merged 26 commits into from
Nov 1, 2024
Merged

Dev -> Main #634

merged 26 commits into from
Nov 1, 2024

Conversation

DonaldKLee
Copy link
Member

No description provided.

Copy link

github-actions bot commented Nov 1, 2024

Visit the preview URL for this PR (updated for commit 27c8285):

https://nwplus-ubc--pr634-dev-dt11h1eo.web.app

(expires Fri, 08 Nov 2024 18:03:20 GMT)

🔥 via Firebase Hosting GitHub Action 🌎

Sign: 8c7ea898e009e43455645bc310bcbccfc0f87e48

Copy link
Contributor

@martincai8 martincai8 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lgtm! just a couple of colour variable things to watch out for

color: ${p => p.theme.colors.bar};
text-align: right;
margin-top: 8px;
`
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

should these colours be theme.colors.text?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

oh idk why it didn't highlight correctly, but i was talking about all the text colours

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

it should be the same color as the bar! it's for the points here above the bar image

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ohhh okay yeah that sounds good! should the the other ones should be colors.text instead of colors.lines?
16065

height: 24px;
border: 2px solid ${p => p.theme.colors.lines};
border-radius: 50%;
color: ${p => p.theme.colors.lines};
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this one too!

Copy link
Contributor

@indirasowy indirasowy Nov 2, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

oh it looks like theme.colors.lines and theme.colors.text both have the same hexcode of #45171A, should I just change it to text so it's more consistent or is it fine to leave it?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yeah i see why that was confusing LOL

it would be amazing if you could change it to text so it matches up with the variable being used in figma! in case those two variables are different for future reskins

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

i'll write the docs for this sometime it's my bad for not specifying it sorry

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ok sounds good! if i push another change do you know if i have to open a new PR because this one was merged?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yepp i can help merge it in

height: 20px;

&::-webkit-progress-bar {
background-color: ${p => p.theme.colors.barBackground};
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

i think design forgot to add variables for these barBackground and bar, please let them know to add it so everything's synced up

@DonaldKLee DonaldKLee merged commit a303d41 into main Nov 1, 2024
3 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants