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

ConnectionModal and ConnectionBanner cleanup #1379

Merged
merged 11 commits into from
Aug 29, 2023

Conversation

masenf
Copy link
Collaborator

@masenf masenf commented Jul 19, 2023

state: save connectError instead of notConnected boolean

Allow connection banner code to access the error message and other details about the websocket error to provide more context to the user.

Add ConnectionModal for displaying connection errors

  • expose connection_error var for downstream use
  • expose has_connection_error var for downstream use
  • expose default_connection_error as list[str | Var] for downstream use with
    other components or styles without duplicating the error message
  • include API_URL in the error message so users can easily see what the frontend
    was trying to connect to

Create rx.connection_modal which takes over the page and definitely does NOT
give any illusion as to the page interactivity status.

default template: add rx.connection_modal()

show off the connection status modal dialog in the default template to help users determine when their backend is disconnected.

fix #1364

image

@masenf masenf changed the title ConnectionModel and ConnectionBanner cleanup ConnectionModal and ConnectionBanner cleanup Jul 20, 2023
Copy link
Contributor

@picklelo picklelo left a comment

Choose a reason for hiding this comment

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

Code looks good, just wondering what the best way for the user to configure this would be. I'm thinking we should have this enabled by default, and let them turn it off.

@@ -15,6 +15,7 @@ class State(rx.State):

def index() -> rx.Component:
return rx.fragment(
rx.connection_modal(), # displays error when backend cannot connect
Copy link
Contributor

Choose a reason for hiding this comment

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

Should we enable this in all apps by default? And have them specify if they want this in the config?

masenf added 3 commits August 17, 2023 16:40
allow connection banner code to access the error message and other details
about the websocket error to provide more context to the user.
* expose `connection_error` var for downstream use
* expose `has_connection_error` var for downstream use
* expose `default_connection_error` as list[str | Var] for downstream use with
  other components or styles without duplicating the error message
* include API_URL in the error message so users can easily see what the frontend
  was trying to connect to

Create `rx.connection_modal` which takes over the page and definitely does NOT
give any illusion as to the page interactivity status.
show off the connection status modal dialog in the default template to help
users determine when their backend is disconnected.

fix #1364
@masenf masenf force-pushed the masenf/connection-banner-cleanup branch from 7f562b2 to 7ab8bd6 Compare August 17, 2023 23:40
@masenf
Copy link
Collaborator Author

masenf commented Aug 17, 2023

Rebased on the new refactored event loop.

masenf and others added 8 commits August 17, 2023 16:42
it doesn't _really_ matter in this context, but we might as well be consistent
with how the var is defined in `useEventLoop`
by default, this is the ConnectionModal, but it could be any component, or None
to disable it entirely
allow overlay_component to be a callable

handle circular import of reflex.config to get the api_url
"Cannot connect to server: ",
connection_error,
". Check if server is reachable at ",
get_config().api_url or "<API_URL not set>",
Copy link
Contributor

Choose a reason for hiding this comment

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

Nit: should we put the url inside an rx.link for easier access?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Yeah and maybe link it to /ping?

@@ -269,6 +275,31 @@ def add_middleware(self, middleware: Middleware, index: Optional[int] = None):
else:
self.middleware.insert(index, middleware)

@staticmethod
def _generate_component(component: Component | ComponentCallable) -> Component:
Copy link
Contributor

Choose a reason for hiding this comment

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

We can move this out of this class also, to a utils file

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I'll do it for the client side routing PR, since I also need to expand the 404 page over there before I pass it to wait_for_redir

@picklelo picklelo merged commit 6b481ec into main Aug 29, 2023
@masenf masenf deleted the masenf/connection-banner-cleanup branch August 29, 2023 22:33
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Include ConnectionBanner in default template and docs
3 participants