Skip to content
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.

Sign up for GitHub

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

Jump to bottom

TraceViewer: Fix show log marker in spanbar #30742

Merged
merged 16 commits into from
Feb 24, 2021
Merged

TraceViewer: Fix show log marker in spanbar #30742

merged 16 commits into from
Feb 24, 2021

Conversation

zoltanbedi
Copy link
Member

Fixes #27885

@zoltanbedi zoltanbedi added this to the 7.5.0 milestone Jan 29, 2021
@zoltanbedi zoltanbedi self-assigned this Jan 29, 2021
@zoltanbedi zoltanbedi requested a review from a team January 29, 2021 14:57
@zoltanbedi zoltanbedi added this to Under review in Observability (deprecated, use Observability Squad) via automation Jan 29, 2021
aocenas
aocenas reviewed Feb 19, 2021
View reviewed changes
Copy link
Member

@aocenas aocenas left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code looks good but there are some visual issues
In dark mode the log marker should probably be some light color to be more visible:
Screenshot from 2021-02-19 15-03-27

The tooltip contents have very little visibility in light theme:
Screenshot from 2021-02-19 15-01-05

packages/jaeger-ui-components/src/TraceTimelineViewer/SpanBar.tsx
@@ -161,7 +161,7 @@ function SpanBar(props: TInnerProps) {
<div
className={cx(styles.wrapper, className)}
onClick={onClick}
onMouseOut={setShortLabel}
onMouseLeave={setShortLabel}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Out of curiosity why this had to be changed?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I had a problem with the tooltip not showing. And after changing that to onMouseLeave it showed up.

From w3schools

Tip: The onmouseleave event is similar to the onmouseout event. The only difference is that the onmouseleave event does not bubble (does not propagate up the document hierarchy).

@zoltanbedi
Copy link
Member Author

Fixed the popover/log marker color problem.

Dark theme:
Screenshot 2021-02-22 at 17 39 40

Light theme:
Screenshot 2021-02-22 at 17 40 44

@zoltanbedi zoltanbedi requested review from a team, thisisobate and natellium and removed request for a team, thisisobate and natellium February 23, 2021 10:18
@zoltanbedi zoltanbedi closed this Feb 23, 2021
Observability (deprecated, use Observability Squad) automation moved this from Under review to Done Feb 23, 2021
@zoltanbedi zoltanbedi reopened this Feb 23, 2021
Observability (deprecated, use Observability Squad) automation moved this from Done to Under review Feb 23, 2021
aocenas
aocenas approved these changes Feb 23, 2021
View reviewed changes
Copy link
Member

@aocenas aocenas left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice work, looks great now.

torkelo
torkelo requested changes Feb 23, 2021
View reviewed changes
Copy link
Member

@torkelo torkelo left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The theme variable choice look strange

packages/jaeger-ui-components/src/TraceTimelineViewer/SpanBar.tsx Outdated
cursor: pointer;
height: 60%;
min-width: 1px;
position: absolute;
top: 20%;
&:hover {
background-color: #000;
background-color: ${theme.colors.textSemiWeak};
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why using text color for a background?

packages/jaeger-ui-components/src/TraceTimelineViewer/SpanBar.tsx Outdated
@@ -65,14 +65,14 @@ const getStyles = createStyle(() => {
`,
logMarker: css`
label: logMarker;
background-color: rgba(0, 0, 0, 0.5);
background-color: ${theme.colors.text};
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This looks wrong , use bg1 or bg2 or bg3 for backgrounds

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

or are these text-like elements?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

not really. I just needed a color that is the reverse of bgcolor. so dark in light theme and light in dark theme.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

can you show what elements in the UI is the logMarker? maybe border2 can work as an alternative

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

These are the log markers
Screenshot 2021-02-23 at 16 21 59

border2 would be light for light which wouldn't work.

@zoltanbedi zoltanbedi merged commit 04a067e into master Feb 24, 2021
Observability (deprecated, use Observability Squad) automation moved this from Under review to Done Feb 24, 2021
@zoltanbedi zoltanbedi deleted the zoltan/span-marker-fix branch February 24, 2021 13:18
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@torkelo torkelo torkelo approved these changes

@aocenas aocenas aocenas approved these changes

@Elfo404 Elfo404 Awaiting requested review from Elfo404

Assignees

@zoltanbedi zoltanbedi

Labels
add to changelog area/tracing type/bug
Projects
Observability (deprecated, use Observ...
  
Done
Milestone
7.5.0-beta1
Development

Successfully merging this pull request may close these issues.

Mark log/event annotations on the Span bar in Trace viewer
4 participants
@zoltanbedi @torkelo @aocenas @Elfo404