-
Notifications
You must be signed in to change notification settings - Fork 419
RI-7740: update instances navigation styles #5206
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
Conversation
| export const ButtonWrapper = styled(Row)` | ||
| cursor: pointer; | ||
| ` |
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.
not sure if this comment is valid but I saw we used role="button" property in other places to achieve this. or i'm missing something
Code Coverage - Frontend unit tests
Test suite run success5325 tests passing in 689 suites. Report generated by 🧪jest coverage report action from 928d546 |
| <Text | ||
| className={styles.breadCrumbLink} | ||
| <Link | ||
| color="subdued" |
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.
In most of the places we go away from this legacy EUI subdued color, is there a reason why we use it here?
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.
yes, the default color of our Link is overrided for some reason. This color is the bluish one
| import { RiIcon } from 'uiSrc/components/base/icons/RiIcon' | ||
| import { Link } from 'uiSrc/components/base/link/Link' | ||
| import InstancesNavigationPopover from './components/instances-navigation-popover' | ||
| import styles from './styles.module.scss' |
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.
Tip: In the last few weeks, I have tried to remove all the SASS styles and migrate them to styled components when I deal with a particular component.
If you find the styles of these components to be simple enough, you can go for it as well.
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 have a dedicated tech debt task on the matter. I'd rather keep this PR short.
I usually do what you are saying as well, but only if I touch more components and half or fewer remain is sass
What
Testing