-
Notifications
You must be signed in to change notification settings - Fork 361
Conversation
CLA Assistant Lite All Contributors have signed the CLA. |
Please also check the spacing between elements. E.g. the gap between copy/share buttons and Total Balance. |
Why no semicircle around arrow that opens the sidebar ser? |
ESLint Summary View Full Report
Report generated by eslint-plus-action |
I think this has to do with cross-browser font-rendering. |
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 good dude. I think you can just abstract some of the values into theme variables.
background-color: ${background}; | ||
|
||
& .icon-color { | ||
fill: #008c73; |
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.
We export this colour as secondary
.
} | ||
|
||
&:hover { | ||
background-color: #effaf8; |
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.
It is probably worthwhile adding this to the variables file.
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 realized we already have it as primaryLite
but I also added primaryGreen200
because of different naming in our style guide. Removed primaryGreen200
again and use primaryLite
now everywhere. We can tackle possible renaming of this in a later task.
background-color: ${background}; | ||
|
||
& .icon-color { | ||
fill: #008c73; |
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.
secondary
} | ||
|
||
&:hover { | ||
background-color: #effaf8; |
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 import this if you add it to the variables file.
transition: background-color 0.2s ease-in-out; | ||
|
||
& .icon-color { | ||
fill: #008c73; |
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.
secondary
src/components/List/index.tsx
Outdated
} | ||
|
||
& .MuiListItemText-root span { | ||
line-height: 1; | ||
} | ||
|
||
&.MuiListItem-button:hover { | ||
background-color: #f6f7f8; |
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.
You're using this colour often as well. I'd export it from the variables file too.
|
Agreed, I changed the style to be like other buttons in the app (16px font-size, not bold) |
REPLACED BY #3778
What it solves
Resolves #3504
How this PR fixes it
How to test it
Open questions
Should some of these changes be done in the
safe-react-components
packageScreenshots