-
Notifications
You must be signed in to change notification settings - Fork 10
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
Differentiate user being offline and API being offline #579
Conversation
Deployed to Cloudflare Pages
|
@@ -277,6 +277,7 @@ | |||
}, | |||
"home": { | |||
"apiOffline": "Our {{ target }} API is offline. We’re trying to reconnect", | |||
"userOffline": "You are offline. We’re trying to reconnect", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@@ -5,9 +5,11 @@ import { AppError, AppErrors } from '../../../types/errors' | |||
import { useFormattedTimestampString } from '../../hooks/useFormattedTimestamp' | |||
import { paraTimesConfig } from '../../../config' | |||
|
|||
export const useIsApiOffline = (network: Network): boolean => { | |||
export const useIsApiOffline = (network: Network): false | 'userOffline' | 'apiOffline' => { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
At this point I think we should rename this to useNetworkProblem()
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think that's too confusing: user's "network" && mainnet "network"?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
isApiReachable?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
isApiReachable?
No, that would make the "false" return value confusing.
How about useConnectionProblem()
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
but it's not necessarily a connection problem if API is unresponsive :D
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
OK, I am fine with useIsApiReachable
, but then we need a different result structure. { reachable: true }
when all is fine, and { reachable: false, reason: "whatever" }
in case of issues.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reviewed, LGTM. (See comment)
<StyledAlert severity="warning">{t('home.apiOffline', { target })}</StyledAlert> | ||
) : null | ||
if (!isNetworkReachable.reachable) { | ||
if (isNetworkReachable.reason === 'userOffline') { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would prefer adding the full text of reason right inside the hook (we have access to translation there, too); no need to have a layer of compilation here...
@@ -139,7 +139,7 @@ export const HomePage: FC = () => { | |||
</LogotypeBox> | |||
<SearchInputContainer> | |||
<SearchInputBox> | |||
<Search disabled={isApiOffline} variant={searchVariant} onFocusChange={onFocusChange} /> | |||
<Search disabled={!isApiReachable} variant={searchVariant} onFocusChange={onFocusChange} /> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we need to update this to get the inside from within the structure returned by the hook.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
😱
No description provided.