feat(trace viewer): link from attach action to attachment tab #33265
Conversation
This comment has been minimized.
This comment has been minimized.
| {showDuration && <div className='action-duration'>{time || <span className='codicon codicon-loading'></span>}</div>} | ||
| {showAttachments && <ToolbarButton icon='attach' title='Open Attachment' onClick={revealAttachments} />} |
There was a problem hiding this comment.
Perhaps put this to the left of the duration? Misaligned durations look ugly.
| const title = <span style={{ marginLeft: 5 }}> | ||
| {linkifyText(attachment.name)} {hasContent && <a style={{ marginLeft: 5 }} href={downloadURL(attachment)}>download</a>} | ||
| const title = <span style={{ marginLeft: 5 }} ref={ref} title={attachment.name}> | ||
| <span style={highlight ? { textDecoration: 'underline var(--vscode-terminal-findMatchBackground)', textDecorationThickness: 1.5 } : {}}>{linkifyText(attachment.name)}</span> |
There was a problem hiding this comment.
We usually add a className conditionally, and then put all styles into a css file.
| </div>; | ||
| })} | ||
| </div>; | ||
| }; | ||
|
|
||
| function isActiveAttachment(attachment: Attachment, activeAction: ActionTraceEventInContext | undefined): boolean { | ||
| return activeAction?.attachments?.some(a => a.name === attachment.name && a.path === attachment.path && a.sha1 === attachment.sha1) ?? false; |
There was a problem hiding this comment.
Perhaps just activeAction?.attachments?.includes(attachment)?
There was a problem hiding this comment.
tried that, but it doesn't work because referential equality is lost in
| @@ -82,9 +82,9 @@ export const TabbedPane: React.FunctionComponent<{ | |||
| tabs.map(tab => { | |||
| const className = 'tab-content tab-' + tab.id; | |||
| if (tab.component) | |||
| return <div key={tab.id} className={className} style={{ display: selectedTab === tab.id ? 'inherit' : 'none' }}>{tab.component}</div>; | |||
| return <div key={tab.id} id={`tab-${tab.id}`} role='tabpanel' title={tab.title} className={className} style={{ display: selectedTab === tab.id ? 'inherit' : 'none' }}>{tab.component}</div>; | |||
There was a problem hiding this comment.
- I am not sure you'd like the whole tabpanel to have this title. I'd expect titles on internal elements of the tab content, not a single one covering the whole panel.
- I am afraid that
ids will clash between various tabbed pane. Should we prefix them with some unique value for thisTabbedPane?
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
| </div>; | ||
| })} | ||
| </div>; | ||
| }; | ||
|
|
||
| function isActiveAttachment(attachment: Attachment, activeAction: ActionTraceEventInContext | undefined): boolean { | ||
| return activeAction?.attachments?.some(a => a.name === attachment.name && a.path === attachment.path && a.sha1 === attachment.sha1) ?? false; |
There was a problem hiding this comment.
- I guess if there are multiple attachments for the
activeAction, you probably want to reveal just the first one? - It seems to me that merely selecting a different action will reveal its attachments? Is that the UI we'd like? I'd prefer highlighting them instead, and only reveal upon an explicit button click.
|
|
||
| const isTextAttachment = isTextualMimeType(attachment.contentType); | ||
| const hasContent = !!attachment.sha1 || !!attachment.path; | ||
|
|
||
| React.useEffect(() => { | ||
| if (highlight) |
There was a problem hiding this comment.
Given that this scrolls, the prop should be called reveal, leaving highlight for visual indication instead.
| onSelect?: (id: string) => void | ||
| }> = ({ id, title, count, errorCount, selected, onSelect }) => { | ||
| onSelect?: (id: string) => void, | ||
| 'aria-controls'?: string, |
There was a problem hiding this comment.
Maybe ariaControls? It is very strange to see a property named like this :)
This comment has been minimized.
This comment has been minimized.
|
I've incorporated your changes by splitting up |
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
| @@ -296,6 +298,10 @@ export const Workbench: React.FunctionComponent<{ | |||
| setSelectedTime={setSelectedTime} | |||
| onSelected={onActionSelected} | |||
| onHighlighted={setHighlightedAction} | |||
| revealAttachment={attachment => { | |||
There was a problem hiding this comment.
You probably want to React.useCallback() here to avoid a new function instance every time.
Test results for "tests 1"5 failed 5 flaky36747 passed, 670 skipped Merge workflow run. |
Adds a button to attach steps that opens up the Attachment tab and scrolls the selected attachment into view.
Screen.Recording.2024-10-24.at.11.02.32.mov
I've added a couple of ARIA props to make this easier to test.