-
Notifications
You must be signed in to change notification settings - Fork 487
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
span bar row size fix #599
Conversation
(cherry picked from commit 3fe952ab8c829072c4e6cd314bfd656c1d1ebfe5) Signed-off-by: Ivan Kopeykin <ikopeykin@ozon.ru>
d83e50d
to
09d763f
Compare
Codecov Report
@@ Coverage Diff @@
## master #599 +/- ##
==========================================
+ Coverage 93.52% 93.58% +0.05%
==========================================
Files 217 217
Lines 5296 5296
Branches 1362 1362
==========================================
+ Hits 4953 4956 +3
+ Misses 302 299 -3
Partials 41 41
Continue to review full report at Codecov.
|
@vankop Can you please attach a gif of hovering over the indent guidelines with your changes? |
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.
Some minor clean up suggestions
content: ''; | ||
padding-left: 3px; | ||
background-color: darkgrey; | ||
background-color: transparent; |
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 believe a psuedoelement without content
is not generated at all.
Please remove this :before
entirely if it is not necessary.
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.
padding is a part of element size
jaeger-ui/packages/jaeger-ui/src/components/TracePage/TraceTimelineViewer/SpanTreeOffset.css
Line 37 in 09d763f
padding-left: 1px; |
:not(:hover)
)
/* The size of the indentGuide is based off of the iconWrapper */ | ||
padding-right: calc(0.5rem + 11px); | ||
border-left: 0px; | ||
border-left: 3px solid darkgrey; |
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 can get away with just border-color: darkgrey
to avoid reproducing the other two characteristics (3px
and solid
)
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 can finally reproduce the issue on master and this seems to have fixed it, ✅
Thanks for figuring out the issue!
(cherry picked from commit 3fe952ab8c829072c4e6cd314bfd656c1d1ebfe5) Signed-off-by: Ivan Kopeykin <ikopeykin@ozon.ru> Signed-off-by: vvvprabhakar <vvvprabhakar@gmail.com>
(cherry picked from commit 3fe952ab8c829072c4e6cd314bfd656c1d1ebfe5) Signed-off-by: Ivan Kopeykin <ikopeykin@ozon.ru> Signed-off-by: vvvprabhakar <vvvprabhakar@gmail.com>
(cherry picked from commit 3fe952ab8c829072c4e6cd314bfd656c1d1ebfe5) Signed-off-by: Ivan Kopeykin <ikopeykin@ozon.ru> Signed-off-by: vvvprabhakar <vvvprabhakar@gmail.com>
Which problem is this PR solving?
resolves #589
Short description of the changes
bug 👇 (black inner bar is custom critical path info, it does not affect a bug)
Steps to reproduce:
What happens:
Not sure why but pseudo element
:before
increases parent's height on:hover
, so here after rendering jaeger thinks that element height changed and fall into recomputing y positions, this happens on each span hover so span height is growing infinitelyI fixed size of
indentGuide
, this actually improves perf a little bit (no layout happens)