Add copy button to structured logs message column#2344
Add copy button to structured logs message column#2344JamesNK merged 14 commits intomicrosoft:mainfrom
Conversation
There was a problem hiding this comment.
- The background color of the copy button doesn't look good. Should it be transparent when not hovered over? That's the same experience as non-error/warning rows, it just so happens the background colors match.
- The exception details icon is on a different line.
dotnet-policy-service
Bot
added
the
needs-author-action
|
Fixed both. |
dotnet-policy-service
Bot
removed
the
needs-author-action
|
Compare with resources page:
|
I made it so the background is only transparent on error.
I do not think the copy button should be at the end of the column. Considering how long the message column can be, it is unlikely to be discoverable. Contrast this with the resources page, which has a small source column width |
…n on error" This reverts commit b626502.
tlmii
left a comment
tlmii
left a comment
There was a problem hiding this comment.
Much cleaner! Thanks!
I do think the "should the copy button be all the way at the end" is worth a conversation before GA but relative to adding the copy button functionality, I don't think its worth blocking the PR at the moment.
There was a problem hiding this comment.
I do not think the copy button should be at the end of the column. Considering how long the message column can be, it is unlikely to be discoverable. Contrast this with the resources page, which has a small source column width
The source column displays it at the end of the column. Also, the property grid has the button at the end of the column, which can be long if displayed horizontally.
For example:
This isn't urgent for P4 and doesn't need to be rushed in. Please make it consistent.
If we want to change copy buttons in columns across the app then that should be separate issue to consider properly.
dotnet-policy-service
Bot
added
the
needs-author-action
|
Friendly reminder that this PR is assigned to the Preview 4 milestone which is snapping in a couple of hours. Please ensure it is merged by 4pm PST in order to ensure the change will be part of Preview 4. |
…tured-logs-message # Conflicts: # src/Aspire.Dashboard/Components/ResourcesGridColumns/LogMessageColumnDisplay.razor
… grid value dispose
|
@JamesNK updated I also added disposal of the fluent tooltip when the grid value is disposed |
dotnet-policy-service
Bot
removed
no-recent-activity
needs-author-action
|
No change to existing buttons |
|
|
||
| public void Dispose() | ||
| { | ||
| _tooltipComponent?.Dispose(); |
There was a problem hiding this comment.
Why is this necessary? There are other places we're using tooltip and not disposing.
There was a problem hiding this comment.
This will close the tooltip. I could only reproduce the issue you filed about the tooltips not closing on data grid data update on tooltips located in the data grid itself. So I don’t think it’s necessary elsewhere
There was a problem hiding this comment.
Which issue is that? I'd like to test it works.
There should be a comment saying why it's being disposed.
There was a problem hiding this comment.
I swear there was an issue that you filed about multiple tooltips appearing when hovering over the copy button & data changes in a data grid, but I cannot find it.
There was a problem hiding this comment.
This one: #2704
Test on the stress project. The resources that update have this behavior.
There was a problem hiding this comment.
I am unable to run the stress project. Starting some of the projects fails
|
The cell browser tooltip shows up:
I added changes to the error icon to fix this happening. Add them to the copy button. |
|
Hidden now |
6 participants
Resolves #2109
word-wrap: nowraphad to be added to GridValue's css as otherwise the content could overflow to another line; this property is beneficial for the other uses of GridValue too, which can suffer from the same problem.Microsoft Reviewers: Open in CodeFlow