chore(html): Reveal elements with Anchor abstraction
#33537
Conversation
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.
packages/html-reporter/src/chip.tsx
Outdated
| > | ||
| {children} | ||
| </Chip>; | ||
| anchorId?: string, |
There was a problem hiding this comment.
I find it strange that the first and only usage is inside AutoChip. I'd prefer to separate them for now, and wrap with Anchor where needed to separate concerns.
There was a problem hiding this comment.
If we want the chip to auto-expand on reveal, then we need to combine them. If not, then i'm happy to separate.
packages/html-reporter/src/chip.tsx
Outdated
| const [expanded, setExpanded] = React.useState(initialExpanded ?? true); | ||
| const onChangeReveal = React.useCallback((isRevealed: boolean) => { | ||
| if (isRevealed) | ||
| setExpanded(true); |
There was a problem hiding this comment.
Why do we need this logic? If it is collapsed by default, perhaps it's fine for it to be collapsed after scrolling into view?
There was a problem hiding this comment.
UX-Wise, I think uncollapsing on reveal saves one click. This might not make a difference for Traces or Videos. But we'll extend this to a specific attachment in the future, and there's no way of revealing an element in a collapsed Chip.
There was a problem hiding this comment.
Thinking about this one, it means in addition to scrolling into view, we might need to perform other actions to reveal, for example expand the chip.
If we think broadly, the way to solve it would be to add revealing logic to various UI elements:
- Make expandable/switchable elements accept
anchorIdthat they willuseAnchor()with and reveal the respective part of the UI. - Separate
Anchorthat will scroll into view from theAutoChipthat will expand, and pass the sameanchorIdto both. - Make
anchorIdhierarchical in the future, so that we can in theory say "AutoChip that contains all attachments has anchorId=attachments/*", and then inside it put individual "Attachment with anchorId=attachments/0".
Let me know what you think.
There was a problem hiding this comment.
You've put into words what I couldn't, yes! That's why useAnchor is probably gonna be exported in the next PR, and why anchorId already accepts a predicate. I'll look at separating the expanding + scrolling logic in AutoChip.
|
|
||
| export function useAnchor(id: AnchorID, onChange: (isRevealed: boolean) => void) { | ||
| React.useEffect(() => { | ||
| const listener = () => { |
There was a problem hiding this comment.
Is this going to trigger if I click the same "reveal" link again? I'd like the item to be revealed even when it's already in the url. And vice versa, I wouldn't want the item to be revealed on mere re-render, e.g. switching a tab in a tabbed pane.
There was a problem hiding this comment.
It triggers for each popstate event, unrelated of re-rendering. As you can see in the video, this includes clicking the link again. If you have a tabbed pane, and that tabbed pane leads to popstate events, then I guess the tabbed pane shouldn't include the anchor param in its links.
This comment has been minimized.
This comment has been minimized.
|
I've implemented the separation we discussed in dc064d4. |
packages/html-reporter/src/links.tsx
Outdated
| }, [id, onReveal]); | ||
| } | ||
|
|
||
| export function Anchor({ id, onReveal, children }: React.PropsWithChildren<{ id: AnchorID, onReveal?(): void }>) { |
There was a problem hiding this comment.
I don't think we need onReveal anymore, let's drop it.
This comment has been minimized.
This comment has been minimized.
Test results for "tests 1"2 flaky36951 passed, 650 skipped Merge workflow run. |
Replaces our custom reveal implementation with an abstracted one.
useAnchorallows subscribing to anchor changes,<Anchor>implementsscrollIntoViewbehaviour.When I demo'd this on Friday, we discussed something about an engage/disengage switch for asynchronous reveals. I don't see a need for that right now, so i've kept it out.
Screen.Recording.2024-11-11.at.11.19.24.mov