feat(ui): update colors to semantic tokens in home and settings folders#16877
Conversation
Bundle ReportChanges will increase total bundle size by 1.99kB (0.01%) ⬆️. This is within the configured threshold ✅ Detailed changes
Affected Assets, Files, and Routes:view changes for bundle: datahub-react-web-esmAssets Changed:
Files in
|
|
✅ Meticulous spotted visual differences in 460 of 1446 screens tested, but all differences have already been approved: view differences detected. Meticulous evaluated ~9 hours of user flows against your PR. Last updated for commit |
|
Linear: CAT-1733 |
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
v-tarasevich-blitz-brain
left a comment
There was a problem hiding this comment.
Looks good to me
Nothing blocking, just a few suggestions/questions
|
|
||
| const Card = styled.div` | ||
| border: 1px solid ${ANTD_GRAY[4]}; | ||
| border: 1px solid ${(props) => props.theme.colors.bgSurface}; |
There was a problem hiding this comment.
use border* colors
|
|
||
| :hover { | ||
| border: 1.5px solid ${SEARCH_COLORS.LINK_BLUE}; | ||
| border: 1.5px solid ${(props) => props.theme.colors.hyperlinks}; |
There was a problem hiding this comment.
borderBrand
| const SubHeader = styled.div` | ||
| font-size: 14px; | ||
| color: ${ANTD_GRAY[5]}; | ||
| color: ${(props) => props.theme.colors.border}; |
There was a problem hiding this comment.
let's try with textTertiary
|
|
||
| > .slick-dots li button { | ||
| background-color: #d9d9d9; | ||
| background-color: ${(props) => props.theme.colors.border}; |
There was a problem hiding this comment.
do we have some more suitable button* token?
|
|
||
| const Card = styled.div` | ||
| border: 1px solid ${ANTD_GRAY[4]}; | ||
| border: 1px solid ${(props) => props.theme.colors.bgHover}; |
There was a problem hiding this comment.
can we use border?
|
|
||
| :hover { | ||
| ${(props) => props.showHover && 'background-color: #f5f7fa;'} | ||
| ${(props) => props.showHover && `background-color: ${props.theme.colors.bgSurface};`} |
| @@ -58,7 +59,7 @@ export default function LinkModule(props: ModuleProps) { | |||
| ellipsis={{ | |||
| tooltip: { | |||
| color: 'white', | |||
There was a problem hiding this comment.
perhaps we can update this white too
|
|
||
| justify-content: center; | ||
| background: ${colors.gray[1600]}; | ||
| background: ${(props) => props.theme.colors.bgSurfaceNewNav}; |
There was a problem hiding this comment.
can we use another tokens not related to new nav?
| margin: 5px; | ||
| border: 1px solid ${colors.gray[100]}; | ||
| border-radius: 12px; | ||
| position: relative; |
There was a problem hiding this comment.
is it safe to remove?
| border-radius: 12px; | ||
| background-color: ${colors.white}; | ||
| background-color: ${(props) => props.theme.colors.bg}; | ||
| ${(props) => props.theme.id === 'themeV2Dark' && 'opacity: 0;'} |
There was a problem hiding this comment.
what do you think if we change it to theme.id !== themeV2Light? I think this background supported only in light theme
Linear ticket:
https://linear.app/acryl-data/issue/CAT-1661/update-colors-to-semantic-tokens-in-settings-and-home-folders
Description:
Updates colors to semantic tokens in the following folders: