Skip to content

Conversation

tlmii
Copy link
Member

@tlmii tlmii commented Nov 14, 2023

This PR replaces #803 that was unexpectedly closed when the repo was made public.

This partially addresses #590. Once this goes in, I'll split off the remaining work from that issue into separate issues that can be picked up by anyone.

  1. The Summary/Details View uses the FluentSplitter, taking advantage of the new Collapsed functionality that was just added in RC3 of the library.
  2. The orientation is remembered between loads of a page and runs of the dashboard
  3. The size is remembered between loads of a page and runs of the dashboard
  4. For 2 and 3, the storage is done with a configurable key. By default this uses the url of the page. But it can be overridden if a) you have multiple summary/details views on one page or b) you want to share the settings between pages. We currently use this so that the three resource list pages (Projects, Containers, Executables) all share the same settings, but Structured Logs and Trace Details have their own (since the summary and details panes are visibly different for those two from the other three). We can adjust that if we want so that they all share a default, all have their own or any other combination.
  5. All five pages with the Summary/Details View are now toggleable. If you click View (or click on the span in Trace Details), and then click on the same one again, the details view will collapse. If you click on a different one, it'll switch the details to that new selection.

Screenshot with the splitter (only real visible change to the user is the gray splitter bar):
image

@tlmii tlmii force-pushed the dev/use-splitter-for-summary-details branch from a8402cf to 3ae63e7 Compare November 14, 2023 21:56
Copy link
Contributor

@smitpatel smitpatel left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

:shipit:

@JamesNK
Copy link
Member

JamesNK commented Nov 14, 2023

I compared how it behaves to the dev tools in browsers.

Feedback:

  • Set a minimum size for each panel. Say, 150px.
  • There is padding around the panel. I think it should extend to the edge of the page. See screenshot below. This might be difficult to fix because of where the padding is specified. Interested in what other people think. I might not it the other way.
  • Position on a page is remembered, but the size isn't remembered.
  • I expect that resizing the browser window would leave the details view in its current position and the area available to rest of the page content would change. Or they change size together but proportionally (down to the min size mentioned above). Current behavior:
    • Size dock: The details view changes size and the main content is unchanged. Bad.
    • Bottom dock: The details view changes size changes proportionally. OK?
  • The splitter bar is very thick. I think the area that can be clicked on should stay the same size, but the visible size should be a lot smaller.

image

@vnbaaij
Copy link
Contributor

vnbaaij commented Nov 14, 2023

@JamesNK I addressed the bottom padding in #823

@JamesNK
Copy link
Member

JamesNK commented Nov 14, 2023

  • I expect that resizing the browser window would leave the details view in its current position and the area available to rest of the page content would change. Or they change size together but proportionally (down to the min size mentioned above). Current behavior:

    • Size dock: The details view changes size and the main content is unchanged. Bad.
    • Bottom dock: The details view changes size changes proportionally. OK?

I tested this more. They panels are resized proportionally if they're resized after they first open. That breaks after they are resized using the splitter.

Demo:

splitter-resize

@tlmii
Copy link
Member Author

tlmii commented Nov 15, 2023

@JamesNK

  • Set a minimum size for each panel. Say, 150px.

I don't see a straightforward way to do this from the outside of the component (though I admit I may have missed something). I filed microsoft/fluentui-blazor#958 to see if it can be a part of the component itself. Will file a follow-up issue to this PR to track it.

  • There is padding around the panel. I think it should extend to the edge of the page. See screenshot below. This might be difficult to fix because of where the padding is specified. Interested in what other people think. I might not it the other way.

@vnbaaij noted this is changed in his PR. If that doesn't fix all of what you're referring to, we can follow up.

  • Position on a page is remembered, but the size isn't remembered.

I found one issue where neither size nor position were remembered on the trace detail page because the URL is part of the key for saving it by default (technically it did save it for the same trace id/span id, but not if you went to a different one). I changed that in this commit. I couldn't find another place where it didn't work. If you still see this, let me know and I'll troubleshoot

  • I expect that resizing the browser window would leave the details view in its current position and the area available to rest of the page content would change. Or they change size together but proportionally (down to the min size mentioned above). Current behavior:

    • Size dock: The details view changes size and the main content is unchanged. Bad.
    • Bottom dock: The details view changes size changes proportionally. OK?

I tested this more. They panels are resized proportionally if they're resized after they first open. That breaks after they are resized using the splitter.

The way the FluentSplitter currently works, when you resize it it gives a fixed width (in px) to the left/top panel and 1fr (essentially "the rest") to the right/bottom panel. So when you resize the page, the left/top always stays the same size. I filed microsoft/fluentui-blazor#957 to suggest adding proportional resize (which is what the SummaryDetailsView component uses) and maybe making the second panel be fixed instead of the first.

@tlmii
Copy link
Member Author

tlmii commented Nov 15, 2023

The splitter bar is very thick. I think the area that can be clicked on should stay the same size, but the visible size should be a lot smaller.

Missed this one earlier. I agree. I played around with styling it a bit in a few different ways, but felt it probably needed some design feedback that warranted its own PR. I'll note this as one of the things to split off to its own issue

@tlmii
Copy link
Member Author

tlmii commented Nov 16, 2023

OK, following up on the above:

  1. Min Width was added to the FluentSplitter component in Splitter enhancements (from Aspire feedback). Fix #957 and fix #958 microsoft/fluentui-blazor#960. I filed Add min-width to Summary/Details View panes #865 to track adding it on our end once that is available.
  2. Proportional sizing was also added to the FluentSplitter component in Splitter enhancements (from Aspire feedback). Fix #957 and fix #958 microsoft/fluentui-blazor#960. We won't need to do anything other than update to that version but I filed Maintain proportional sizing in FluentSplitter on browser resize #866 as a placeholder for that issue on our end.
  3. Even though Add a small footer #823 (at least partially) addresses the padding issue, I filed Remove padding around Summary/Details view #867 to track it to make sure it gets addressed if that PR doesn't get taken as-is.
  4. I filed Restyle the splitter bar in Summary/Details View #864 to track restyling the splitter bar as a separate issue since I figure that'll go through some design iterations and don't want to delay this PR further.

I was not able to find a case where the splitter didn't remember its size or position after I made the one fix above. If anyone does find a case where that happens, I'll track it down.

@tlmii tlmii merged commit 8203451 into dotnet:main Nov 16, 2023
@tlmii tlmii deleted the dev/use-splitter-for-summary-details branch November 16, 2023 06:32
private async Task HandleDismissAsync()
{
await OnDismiss.InvokeAsync();
}

private void HandleToggleOrientation()
private async void HandleToggleOrientation()
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

async Task

To be honest, I'm not sure what Blazor does with the task, but async void is rarely good.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

D'oh. Looks like I let VS add the async for me when I typed await but then there were no errors/warnings so I forgot to go back and change void to Task too. Thanks for the catch. Pushed a fix in #883

@github-actions github-actions bot locked and limited conversation to collaborators Apr 29, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants