-
Notifications
You must be signed in to change notification settings - Fork 3.5k
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
feat(trace-viewer): display text attachments in ui mode #31215
feat(trace-viewer): display text attachments in ui mode #31215
Conversation
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
@alvaromartmart Thank you for the PR! We discussed this with the team, and came up with a few comments/suggestions:
|
This comment has been minimized.
This comment has been minimized.
…xes. This commit will likely be reverted
This comment has been minimized.
This comment has been minimized.
Thank you for the feedback!
Here's a (WIP) screenshot with some annotations (more on them below) ![]()
Agree on the "no preview available". Additionally, I was wondering whether we should
They seem to render just fine, do you have a specific edge case in mind?
The screenshot above is the result of some experimentation using the
|
This comment has been minimized.
This comment has been minimized.
All good questions - the lack of definitive answers to those is the exact reason we did not rush into implementing it. If you have a good idea on reasonable defaults, include those in the PR and we'll be happy to take a look. |
This comment has been minimized.
This comment has been minimized.
Ok so in 0eb57e7 I placed the attachment in an expansion panel. You can see a demo here Screen.Recording.2024-06-25.at.23.21.58.movThis way:
As for the Final question: since it's now using an expansion panel, maybe I can get rid of the |
This comment has been minimized.
This comment has been minimized.
ugh, forgot to update the tests - tomorrow |
This comment has been minimized.
This comment has been minimized.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Your change looks good. I have a code suggestion inline and a UX suggestion below:
- Expandable title should not click away, the entire title should be a click target for expand, that would make it a better UX
- You can have a download icon next to it to allow downloading, both in expanded and collapsed state
const [text, setText] = React.useState<string | null>(null); | ||
|
||
React.useEffect(() => { | ||
let isMounted = true; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Instead of this, I wonder if we should change the <Expandable>
contract to be
<Expandable expanded={} body={() => <TextAttachment/>}>
We don't seem to have any usages of it yet, so easy to change.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Instead of this, I wonder if we should change the contract to be
<Expandable expanded={} body={() => <TextAttachment/>}>
Not a React expert but I'm not sure I really like this change in contract vs. directly "projecting" the body into the Expandable
component.
We don't seem to have any usages of it yet, so easy to change.
The Expandable
was already used here, though it doesn't rely on the body, it seems it's only used to toggle the state.
All things considered, I can apply the change if you want as it seems quite straightforward one. Just not sure it's worth it. WDYT?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It would look better than the manual isMounted management to me!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Got rid of the isMounted
as part of #31215 (comment), but I'm not sure if your request here is still relevant.
On a different note, I wonder if at this point I should merge ExpandableAttachment
and TextAttachment
into a single component for simplicity. Please LMKWYT
Done, thanks for the feedback: 700d061 Demo: Screen.Recording.2024-06-28.at.23.16.05.mov |
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
In 677ecae I slightly simplified & improved the |
This comment has been minimized.
This comment has been minimized.
Test results for "tests 1"8 flaky28408 passed, 655 skipped Merge workflow run. |
Closes #30976
Displays text attachment contents in UI mode