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.

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

Add Duration/Size columns to the header of view-event list. #55

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

Haoxiqiang
Copy link

image

The request list shows the duration/size may be more intuitive for analysis.

@CLAassistant
Copy link

CLAassistant commented Sep 22, 2022

CLA assistant check
Thank you for your submission! We really appreciate it. Like many open source projects, we ask that you sign our Contributor License Agreement before we can accept your contribution.
You have signed the CLA already but the status is still pending? Let us recheck it.

Copy link
Member

@pimterry pimterry left a comment

Choose a reason for hiding this comment

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

Hi @Haoxiqiang, thanks for this! Great move 👍 👍 👍

In general I'm totally on board with this as a feature, it makes perfect sense, but I know some users already have issues with the limited UI space in the view list, and this would make that worse.

I think this would be fine if it was optional though. Would you be up for adding that? I'm imagining a little ... button in the top right of the headers, where you can toggle columns on & off. I would suggest only including these two new columns in the options there, not the existing ones for now, as toggling the built-in columns may be quite a bit more complicated (due to some new rows with non-matching layouts - see EventTypeColumn, RTCEventLabel, BuiltInApiRequestDetails etc).

Does that make sense?

(durationMs < 100) ?
(sigFig(durationMs, 2) + 'ms') :
(durationMs < 1000 ? sigFig(durationMs, 1) + 'ms' : (durationMs < 10000 ? sigFig(durationMs / 1000, 3) + ' s' : sigFig(durationMs / 1000, 1) + ' s'))
)
Copy link
Member

Choose a reason for hiding this comment

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

I think this whole chunk would probably be nicer as a reusable function like formatDuration somewhere, e.g. src/util/index.ts.

@Haoxiqiang
Copy link
Author

Haoxiqiang commented Sep 24, 2022

ok, I'll do it later.

@Haoxiqiang
Copy link
Author

Refactored the code.

Most of the time, the space is actually enough, even if it is not enough, the path part can be compressed.
A button on the right side of this may not be pretty, I'll think about how to do it.

@pimterry
Copy link
Member

Great, thanks for refactoring that 👍.

A button on the right side of this may not be pretty, I'll think about how to do it.

To be clear, I'm imagining something like this:

Screenshot from 2022-09-28 15-43-28

When clicking those dots, it can just open a little menu beneath with checkboxes, something like:

Show extra columns:

[ ] Duration
[ ] Response body size

I'm sure you're right that on a normal screen it fits fine in most cases, but there's definitely lots of users with much smaller screens, and at 1024x768 it's already all pretty squeezed!

guyyaffear pushed a commit to Colman-Cyber-Internship/pipe-ui that referenced this pull request Jun 4, 2023
…_yarn/terser-4.8.1

Bump terser from 4.8.0 to 4.8.1
@pimterry pimterry force-pushed the main branch 2 times, most recently from e0e350c to 1d713c8 Compare May 7, 2024 20:22
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants