-
Notifications
You must be signed in to change notification settings - Fork 4
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
UI updates #636
UI updates #636
Conversation
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.
LGTM minus a few nits
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.
CHUNGUS CHANGES WE LOVE A DESIGNER THAT CAN CODE. Just some small nits here and there but the biggest thing is to create a "theme" with the styling we write. So instead of specifying 1px
for borders or for spacing in multiple components, we can just define it within a file and call 3xs
for example. This standardizes our code, so that when we get designs from you guys, we can even just say that a border/padding/spacing is simply s, m, l
etc. You can add to this file here.
packages/frontend-v2/components/AddCourseModal/AddCourseButton.tsx
Outdated
Show resolved
Hide resolved
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
300: "#f4f6f9", | ||
400: "#7586a0", | ||
700: "#e2e6ec", | ||
900: "#d2d8e2", |
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.
300 and 700 arent used, 900 is basically gray2, and changed 400 to gray4
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.
Just a small nit, once you change it you can merge! 💯
700: "#e2e6ec", | ||
800: "#1f242c", | ||
900: "#d2d8e2", | ||
gray0: "#f4f6f9", |
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.
Can you just change these to gray.0
. So like gray would be its own object. This way we aren't doing gray0
, gray1
. It would instead be gray.50
, or gray.100
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.
Ohhh gotcha i see what you mean now
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.
Would it be fine to just do neutral.100
instead of neutral.gray.100
since neutral is just gonna be for grays?
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.
Yea if we feel like there wouldn't be anymore neutrals, then this works. Just saw the changes, you can merge whenever
Description
Made the following UI modifications:
ScheduleTerm
lg
input sizes tomd
for consistencySectionRequirement
textAlso updated the
primary.neutral
colors in the theme folder to remove unused grays and replace some with the names used in figma (gray0, gray, gray2, gray3).primary.neutral.main
was replaced withprimary.neutral.gray
.Old:
New:
Type of change
Please tick the boxes that best match your changes.
yarn install
yarn dev:migration:run
How Has This Been Tested?
Please describe how you tested this PR (both manually and with tests) Provide instructions so we can reproduce.
Checklist: