Move more info from the tap table into the expanded row#1641
Conversation
web/app/css/tap.css
Outdated
|
|
||
| & hr { | ||
| color: red; | ||
|
|
There was a problem hiding this comment.
oooops. also that color: red should not be in here 😂
There was a problem hiding this comment.
good catch! 😂 I didn't even realize that.
dadjeibaah
left a comment
dadjeibaah
left a comment
There was a problem hiding this comment.
This looks great, thanks for making the change, had one nit but should be good to go after that 📦
siggy
left a comment
siggy
left a comment
There was a problem hiding this comment.
looks great! a couple tioli ui comments...
web/app/css/tap.css
Outdated
| padding-right: 50px; | ||
|
|
||
| & .tap-info-section { | ||
| padding: 24px 0; |
There was a problem hiding this comment.
the expanded section feels a bit spaced out, what do you think about something like:
padding: 2px 0;
There was a problem hiding this comment.
it's true! I'll make it more compact. how about 4px 0 :P?
web/app/css/tap.css
Outdated
|
|
||
| & hr { | ||
| color: red; | ||
|
|
There was a problem hiding this comment.
the hr elements don't quite extend to the width of the text. what do you think about something like:
padding: 0;
i noticed the mocks have the hr's going all the way across, that looks nice too.
There was a problem hiding this comment.
yeahhh the reason I have that padding there is because of the + / - that we're using from the ant expandable table row, which causes this expanded section to be 50px from the left.
We could accordion all these rows, but for now I've just made the padding symmetric. We're gonna move off the ant tables soon so it won't matter then either!
|
Actually, have one more comment, It seems like the authority text wraps when the text is too long. We probably could move the "Path" section over 1 or two grid lengths Something like: <React.Fragment>
<h3>Request Init</h3>
<Row gutter={8} className="expand-section-header">
<Col span={8}>Authority</Col>
<Col span={8}>Path</Col>
<Col span={2}>Scheme</Col>
<Col span={2}>Method</Col>
</Row> |
|
What do you think of moving the TLS info into the expanded row as well? |
klingerf
left a comment
klingerf
left a comment
There was a problem hiding this comment.
This is a HUGE improvement. I just had one comment about JS undefined errors, but otherwise looks good to me.
| </Row> | ||
| <hr /> | ||
| <Row gutter={8} className="tap-info-section"> | ||
| {responseInitSection(d)} |
There was a problem hiding this comment.
I'm not sure if this was a problem in the existing implementation or not, but I tried to expand a row for a request that had not finished yet, and I encountered the following JS error:
I think the right fix here is to only add the response init and response end rows if the responseInit and responseEnd objects are present.
klingerf
left a comment
klingerf
left a comment
There was a problem hiding this comment.
⭐️ This looks great! Thanks for adding the latency column back.
* master: Move more info from the tap table into the expanded row (linkerd#1641) `linkerd check` sends params on version check (linkerd#1642) Bikeshed the tap and top icons (linkerd#1637) Add link to tap each row in top table (linkerd#1643) Bump default check retry time to 5 minutes (linkerd#1645) Make wait=true a default option for check and dashboard (linkerd#1640) Add version check to Grafana dashboard (linkerd#1638) Add data plane check for metrics Prometheus (linkerd#1635)
5 participants
Try to make the tap table easier to parse by moving some info into the expanded row.
You can also now click anywhere on the row to expand.
The mocks in #1629 have Authority and Path buried, but I figured they might be useful to see in the top level, so I added them there. I also added http and grpc status so the user can still get a sense of responseInit and responseEnd. Comments on column choice welcome.
Fixes #1629