-
Notifications
You must be signed in to change notification settings - Fork 484
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
Use JSON for table values only for objects #2168
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #2168 +/- ##
=======================================
Coverage 96.57% 96.57%
=======================================
Files 254 254
Lines 7620 7629 +9
Branches 1921 1925 +4
=======================================
+ Hits 7359 7368 +9
Misses 261 261 ☔ View full report in Codecov by Sentry. |
0187d1c
to
ad79155
Compare
I actually like that it shows numbers differently |
I can understand the improvement from the numbers having a different color, but a random offset is not something you see in any software showing tables. It just looks broken to me. |
I think the way to fix the offset would be via changing the styling, not the business logic. |
packages/jaeger-ui/src/components/TracePage/TraceTimelineViewer/SpanDetail/KeyValuesTable.tsx
Show resolved
Hide resolved
ad79155
to
8b0132e
Compare
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.
Thanks!
looks like the existing test cases are missing a case for boolean value, can you add it? |
Without this numbers are treated as json, which styles them with an offset. Signed-off-by: Ivan Babrou <github@ivan.computer>
8b0132e
to
fba6b14
Compare
Sure, added. |
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.
🎉
Without this numbers are treated as json, which styles them with an offset:
With this change applied:
It's possible that there's a better solution for this.
Which problem is this PR solving?
https://github.com/jaegertracing/jaeger-ui/pull/1724/files#r1491924866
How was this change tested?
Manually tested.
Checklist
jaeger
:make lint test
jaeger-ui
:yarn lint
andyarn test