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

home: Stop assuming account existence from loading page #1235

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

PIG208
Copy link
Member

@PIG208 PIG208 commented Dec 30, 2024

We could pass realmUrl when initializing the _LoadingPlaceholderPage, but that will still require a check for the existence of the account.

The loading page will be blank when the account does not exist. The user can't easily reach this page because they can only logout from ChooseAccountPage, until we start invalidating API keys. Even then, they will only see the blank page briefly before getting navigated, so we should not show any text at all.

Screenshot

image

Fixes: #1219

We could pass realmUrl when initializing the `_LoadingPlaceholderPage`,
but that will still require a check for the existence of the account.

The loading page will be blank when the account does not exist. The user
can't easily reach this page because they can only logout from
`ChooseAccountPage`, until we start invalidating API keys. Even then,
they will only see the blank page briefly before getting navigated, so
we should not show any text at all.

Fixes: zulip#1219

Signed-off-by: Zixuan James Li <[email protected]>
@PIG208 PIG208 added the maintainer review PR ready for review by Zulip maintainers label Dec 30, 2024
@PIG208
Copy link
Member Author

PIG208 commented Dec 30, 2024

Not sure if we still show the loading indicator there, though. This page can be visible for a frame only after #737, but if we don't have the realmUrl, there will potentially be a layout shift as the circular indicator's position is affected by the hidden text.

Because realmUrl probably stays the same for the same accountId, an alternative fix would be caching the realm URL when we first learn about the account, and pass that down. This requires us to know for sure that the account exists at some point, unless we allow the realmUrl to be null for the loading page.

@gnprice
Copy link
Member

gnprice commented Dec 30, 2024

Not sure if we still show the loading indicator there, though. This page can be visible for a frame only after #737, but if we don't have the realmUrl, there will potentially be a layout shift as the circular indicator's position is affected by the hidden text.

Yeah, that's a fine reason to leave out the loading indicator. It's probably less jarring to have it disappear while the page is animating out than to have it suddenly shift while the page is animating out.

Note it's more than a frame — the animation duration for leaving a page is more like 200ms.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
maintainer review PR ready for review by Zulip maintainers
Projects
None yet
Development

Successfully merging this pull request may close these issues.

ui: Incorrect null-check operator in _LoadingPlaceholderPageState.build
3 participants