feat(html): link from attachment step to attachment #33267
Conversation
This comment has been minimized.
This comment has been minimized.
|
demo: Screen.Recording.2024-10-24.at.14.40.25.mov |
This comment has been minimized.
This comment has been minimized.
| <span style={{ float: 'right' }}>{msToString(step.duration)}</span> | ||
| {attachmentName && <a style={{ float: 'right' }} title='link to attachment' href={'#' + componentID(params => params.set('attachment', attachmentName))} onClick={(evt) => { evt.stopPropagation(); }}>{icons.attachment()}</a>} |
There was a problem hiding this comment.
Does this rely on the browser to automatically scroll to the anchor? How does that work together with the router? I am confused.
There was a problem hiding this comment.
Yes, that's how it works. Didn't even know we had a router until now - i'll see if I can switch this to our Link component.
I figured using the anchor to target an element is the most browser-standards way of making this work. But I can also change it to use scrollIntoView if you'd prefer that.
There was a problem hiding this comment.
#33295 is a refactoring that allows us to use one scrollIntoView implementation for all attachments. It makes this implementation a lot easier.
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.
|
I've updated this PR to use the |
This comment has been minimized.
This comment has been minimized.
|
sorry, i'll need to look more into this to make links to |
|
Here's a demo of it working: Screen.Recording.2024-11-20.at.10.15.00.mov |
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
| }, [result]); | ||
|
|
||
| const screenshotAnchor = React.useMemo(() => screenshots.map(a => `attachment-${a.name}`), [screenshots]); |
There was a problem hiding this comment.
screenshotAnchors?- why not return this from the previous
useMemo()?
Same for otherAttachmentsAnchor.
There was a problem hiding this comment.
That'd also work, i'll make that change.
| <span style={{ float: 'right' }}>{msToString(step.duration)}</span> | ||
| {attachmentName && <a style={{ float: 'right' }} title='link to attachment' href={`#?testId=${test.testId}&anchor=attachment-${encodeURIComponent(attachmentName)}&run=${test.results.indexOf(result)}`} onClick={evt => { evt.stopPropagation(); }}>{icons.attachment()}</a>} |
There was a problem hiding this comment.
- I wonder why do we use
<Link>in some places, and plain<a>in others? - Can we simplify link generation to avoid passing
testandresultaround somehow? Perhaps take existing ones from the current url? Or put them into a context? Something for a followup.
There was a problem hiding this comment.
- no idea 🤷 I guess I can also rewrite this to
<Link>, but I don't think there'll be a big difference since it's not a text link but a button - agree, that'd be nice. i'll look at it in a followup.
There was a problem hiding this comment.
i've tried rewriting, but that doesn't work as well because <Link> doesn't allow passing in the float: 'right' style. Let's stick with <a>.
Test results for "tests 1"1 failed 1 flaky37139 passed, 650 skipped Merge workflow run. |
Similar to #33265. Adds a link from the attachment step to the attachment view.