Implement Timestamp/Duration columns in view-event-list.tsx #174
Conversation
|
|
|
Removed the default columns and Size column from the list for now. Will implement showing/hiding them in a separate PR. 
|
There was a problem hiding this comment.
Very nice work, thanks @Sugavanas! Overall this looks great to me. I have a wide sprinkling of small things we'd need to finish up to merge which I've noted in here, but nothing major. I've been meaning to get around to this for a long while and never quite found the time, thanks so much for digging into it!
| <RowMarker role='cell' category={category} title={describeEventCategory(category)} /> | ||
| <Timestamp role='cell' /> //TODO: Expose timingEvents |
There was a problem hiding this comment.
There's a leftover TODO in here that gets rendered into the UI 😄
If you want to test this, the easiest option is to launch Chromium (not Chrome, which blocks interception nowadays, so you might need to install it) and then use https://webrtc.github.io/samples/src/content/datachannel/basic/
If it's too complicated to show this for RTC, we can handle that as a separate step later as long as the resulting layout is something reasonable for now. Nice to have but RTC support definitely isn't critical for this (it's rarely used, and exact timing is usually much less relevant).
| <RowMarker role='cell' category={category} title={describeEventCategory(category)} /> | ||
| <Timestamp role='cell' /> //TODO: Expose timingEvents |
| ${columnStyles}; | ||
| transition: flex-basis 0.1s; | ||
| flex-shrink: 0; | ||
| flex-grow: 0; |
There was a problem hiding this comment.
This needs to have a fairly fixed width (either explicitly, or by using fixed width numerals). Right now, different numbers are different widths. Since this is the first column, that breaks alignment for the rest of the row, like so:
We have similar problems in the request counter at the bottom - I think we solve that by using the monospaced font there.
| if (uiStore) { | ||
| const { visibleViewColumns } = uiStore; | ||
| renderComponent = visibleViewColumns.get(columnName) === true | ||
| } |
There was a problem hiding this comment.
UiStore will always be provided here (the ? is just because of the types) so you can use uiStore! instead to skip the if and simplify this.
| get themeName() { | ||
| return this._themeName; | ||
| } | ||
|
|
||
| get visibleViewColumns() { | ||
| return this._visibleViewColumns | ||
| }; |
There was a problem hiding this comment.
Since the value is exactly the same as the field, I think we can drop the getter and just make the field public like expandedViewCard and similar.
| private _visibleViewColumns = new Map<string, boolean>([ | ||
| ['Timestamp', false], | ||
| ['Duration', true] | ||
| ]); //TODO: implement a strict type |
There was a problem hiding this comment.
Would be good to do this TODO, and I think it's fairly easy:
- Export a type of hideable column name strings from here, and use that in this
- Make this a { [name: HideableColumnName]: boolean } object so it doesn't allow other keys
- Import & use
HideableColumnNameinColumnVisibilityToggle
Does that work?
| duration < 5000 ? sigFig(duration / 1000, 2) + 's' : // 3.04s | ||
| duration < 59000 ? sigFig(duration / 1000, 1) + 's' : // 30.2s | ||
| sigFig(duration / 60000, 1) + 'm' // 1.1m |
There was a problem hiding this comment.
This is used in various other contexts, including inside sentences in the cache explanations details, and it would be better to keep it with full words for readability in that case (seconds not s). Can we preserve the previous behaviour but use a 'short' flag or something similar?
| return null; | ||
|
|
||
| return formatDuration(duration); | ||
| }; |
There was a problem hiding this comment.
I'd rather not making a util/utils file, since it's very very non-specific and it'll just become a dumping ground for unrelated stuff. We could have a utils/time, or this could just go in utils/text (since it's really just formatting data into text) or something else, up to you. Should probably go with formatDuration in all cases though since they're closely related.
| @@ -142,4 +144,28 @@ export class ViewEventContextMenuBuilder { | |||
| }; | |||
| } | |||
|
|
|||
| getHeaderToggleContextMenu(enabledColumns: Map<string, boolean>) { | |||
There was a problem hiding this comment.
I don't think this should take any arguments. This class is already storing the uiStore, it should just read the current columns and set them directly. That also means we don't need the onHeaderColumnOptionChange constructor argument either.
|
|
||
| export type FormattedDurationProps = | ||
| { | ||
| durationMs?: number, |
There was a problem hiding this comment.
It looks like nothing ever sets this nowdays (since the timingEvents prop isn't optional here, it would be quite awkward to do so) so we can drop the durationMs prop entirely, and simplify this: calculateAndFormatDuration and DurationPill can just always take TimingEvents and nothing else.
This request adds support for httptoolkit/httptoolkit#90