Skip to content
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

Avoid reconnection modal taking up entire screen, not being able to i… #3189

Merged

Conversation

adamint
Copy link
Member

@adamint adamint commented Mar 26, 2024

Fixes #2514

This isn't the prettiest, please share your UI ideas if you have any, but this PR makes the reconnection modal centered, smaller, not see-through, and allows you to interact with the rest of the page.

I didn't want to remove the spinner nor the reconnection number count so overrode the existing styling of the ui container with !important, though we could also just create the container ourselves and hook into Blazor reconnection events to update the content.

image
image

Microsoft Reviewers: Open in CodeFlow

Copy link

@leslierichardson95 leslierichardson95 left a comment

Choose a reason for hiding this comment

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

This looks good to me. While I kinda wish that the rest of the page was dimmed a bit, I understand the desire to be able to interact with the rest of the page so that's fine with me.

@tlmii
Copy link
Member

tlmii commented Mar 26, 2024

In my mind there's a disconnect between this being (essentially) a dialog that covers part of the page when the page is not interactive and that dialog not being modal that makes me a little uneasy about this.

I get, though, that what we're trying to achieve here is a bit atypical. Most interaction with the page won't work. You can't click internal links, can't open settings, can't pick a different resource/metric/etc, can't expand the details panels, can't even scroll the logs pages since they're virtualized. So you're limited to reading and manually copying text from whatever information happen to be on the screen at the moment. And we can't really allow the latter without also allowing the former (or more accurately allowing the former actions to be attempted, since it won't work).

There's a tension between "making sure the user realizes why nothing they're clicking on is working" and "keep it out of the way of the user for when they need to read/copy info". If the latter were more important I'd say a yellow-bar style notification would be better. But I think that's actually the less common scenario.

You've done a good job of getting the dialog message out of the way of the content so the user can read/copy what they need while still making it obvious. I might tweak it a little bit (perhaps a very small shadow to separate it from underneath - particularly in the light mode - since there's no gray backdrop?; maybe a bit of internal padding for when the page gets small - see screenshot below?). But overall I think this is a fine improvement for the use case we're concerned about here.

image

@premun
Copy link
Member

premun commented Mar 29, 2024

FWIW, I logged the original issue because I was unable to scroll to an exception in the log. So if we still have an invisible overlay that prevents interacting with the elements behind it, it's a little bit better but not quite solves the problem.

I agree with @tlmii that a yellow bar on top would be superior.

@adamint
Copy link
Member Author

adamint commented Mar 29, 2024

@premun there is no longer an invisible overlay with this PR. I believe @tlmii is saying the opposite, that he prefers this approach over a yellow bar. Could you confirm that, Tim? or whether I am misunderstanding 😄

FWIW if we want to do a yellow bar, we can do that pretty easily

@tlmii
Copy link
Member

tlmii commented Mar 29, 2024

@adamint @premun On balance, I think the dialog approach Adam has is better because (in my opinion) "Ensuring people understand why they can no longer interact with the page" is more important than "ensuring the page is not obstructed at all" given how little interactivity is available while trying to reconnect.

@premun To clarify something that I may not have been clear on - in the implementation in this PR, there is no overlay that blocks interaction. But since this is (non-WASM) Blazor, and we're in the disconnected/trying to reconnect state, you can't interact with anything that isn't purely client side anyway. For instance, in your most recent synopsis of the issue "I was unable to scroll to an exception in the log" - you won't be able to scroll to the exception anyway (yellow bar or dialog) because the logs are virtualized and that requires the connection back to the server. If its already on the screen, you'd be able to read it and copy/paste it, so its good that this PR doesn't obstruct much, if any, of the log viewer.

@drewnoakes
Copy link
Member

How does this look in dark theme?

@adamint
Copy link
Member Author

adamint commented Apr 3, 2024

image

dark theme @drewnoakes, same background color as before in dark theme 😄
image

@adamint adamint enabled auto-merge (squash) April 3, 2024 19:28
@adamint adamint merged commit 87fab1e into dotnet:main Apr 3, 2024
8 checks passed
adamint added a commit to adamint/aspire that referenced this pull request Apr 8, 2024
dotnet#3189)

* Avoid reconnection modal taking up entire screen, not being able to interact with page

* add slight padding, shadow

---------

Co-authored-by: Adam Ratzman <adamratzman@microsoft.com>
(cherry picked from commit 87fab1e)
RussKie pushed a commit that referenced this pull request Apr 9, 2024
* Make sure copy button is tab-accessible and visible on tab (#3192)

Co-authored-by: Adam Ratzman <adamratzman@microsoft.com>
(cherry picked from commit 2bba2ad)

* Avoid reconnection modal taking up entire screen, not being able to i… (#3189)

* Avoid reconnection modal taking up entire screen, not being able to interact with page

* add slight padding, shadow

---------

Co-authored-by: Adam Ratzman <adamratzman@microsoft.com>
(cherry picked from commit 87fab1e)

* Force log level select position to be below (#3406)

Co-authored-by: Adam Ratzman <adamratzman@microsoft.com>
(cherry picked from commit 5a6856d)

* Re-focus view button when closing details panel (#3368)

* Re-focus button when closing details panel

* Add internal id for log entries, make sure to null element id on detail view closed

* Make sure view button is selected only after a user action

---------

Co-authored-by: Adam Ratzman <adamratzman@microsoft.com>
(cherry picked from commit e784c91)
@github-actions github-actions bot locked and limited conversation to collaborators May 4, 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.

Attempting to reconnect modal should not be an overlay?
5 participants