-
Notifications
You must be signed in to change notification settings - Fork 414
RI-7691: Polish Profiler styles #5139
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
Code Coverage - Frontend unit tests
Test suite run success5230 tests passing in 681 suites. Report generated by 🧪jest coverage report action from 2cb1b26 |
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.
I think you should be provided with a dark theme svg 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 should stirve to use one image for both, if possible
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.
IMO, we shouldn't block the PR with this. If we have such an image in the future, we'll address it in a separate one.
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 should stirve to use one image for both, if possible
| import MonitorLog from '../MonitorLog' | ||
| import MonitorOutputList from '../MonitorOutputList' | ||
|
|
||
| import ProfilerImage from 'uiSrc/assets/img/profiler/magnifier.svg' |
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.
Why not import it as SVG component and output as such?
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 I got you, but check 04bd33a
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.
What I meant is:
import ProfilerImage from 'uiSrc/assets/img/profiler/magnifier.svg?react'
// ---
<ProfilerImage
alt="Profiler"
style={{ userSelect: 'none' }}
/>Basically how icons work now
| data-testid="monitor-not-started" | ||
| > | ||
| <StyledImagePanel align="center"> | ||
| <StyledImage as="img" src={ProfilerImage} alt={'asd'} /> |
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.
why do you need as? Also, my comment above stands
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.
Because I was using <img /> instead of <RIImage /> and TS complained about as.
Currently, the icon that should be in front of the result text is missing, so I added the most suitable one, IMO, from the existing set.