-
Notifications
You must be signed in to change notification settings - Fork 3
Global style #9
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
Global style #9
Conversation
…tried to fix some of the affected styles
florisdobber
left a comment
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.
Looks great to me, super helpful! Let Jack take a look to see if the React makes sense.
florisdobber
left a comment
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.
Perfect I was looking into doing this but couldn't figure it out
jackblanc
left a comment
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.
Love the progress with this. There are a few things we'll want to change to use https://styled-components.com/ in the future, but we have to merge frontend-scaffold first.
jackblanc
left a comment
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 looks pretty good, there's a few places where we want to use styled-components now. Sorry that the directions changed with that. Reach out on Slack with questions!
|
Changed some places to use styled components, no longer override antd styles and removed inline magic color constants. Did not change login and signup input containers because I believe that might be a separate ticket that involves reorganizing those components a bit more heavily. |
| height: 100%; | ||
| width: 100%; | ||
| background: white; | ||
| background: @white; |
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.
🔥
jackblanc
left a comment
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.
Looks great!
Why
Monday.com Ticket
What benefit does this bring to the end user? Or, what benefit does this bring to developers working in the codebase?
This allows developers to use antd components like text and buttons with the automatic styles that are used throughout the application
This PR
Describe the changes required and any implementation choices you made to give context to reviewers.
All of the variables are located within the craco.config.js file. The colors are at the top of the page and they correspond to different greens and greys that exist in the figma designs. Any of these values can be overridden in less files, see the info-card.less file for an example of overriding antd global styles.
Screenshots
Provide screenshots of any new components, styling changes, or pages.
Here is what one of the template page looks like with the new theme. No code or styling was changed on this page, the new colors come from the global style
Verification Steps
What steps did you take to verify your changes work? These should be clear enough for someone to be able to clone the branch and follow the steps themselves.
Checked the styling on all of the current pages, removed instances of p and h from application to ensure styling works everywhere.