Skip to content

api-fetch: Check navigator.onLine to improve failure notices #71438

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.

Sign up for GitHub

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

Merged
mcsf merged 3 commits into from
Sep 1, 2025
Merged

api-fetch: Check navigator.onLine to improve failure notices #71438

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
073b766
Check navigator.onLine to improve failure notices
mcsf Aug 29, 2025
aff32be
Address feedback
mcsf Sep 1, 2025
b0c0b85
Update tests
mcsf Sep 1, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
18 changes: 15 additions & 3 deletions packages/api-fetch/src/index.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -114,11 +114,23 @@ const defaultFetchHandler: FetchHandler = ( nextOptions ) => {
throw err;
}

// Otherwise, there is most likely no network connection.
// Unfortunately the message might depend on the browser.
// If the browser reports being offline, we'll just assume that
// this is why the request failed.
if ( ! window.navigator.onLine ) {
throw {
code: 'offline_error',
message: __(
'Unable to connect. Please check your Internet connection.'
Copy link
Member

@tyxla tyxla Sep 1, 2025

Choose a reason for hiding this comment

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

I was unsure about the capitalization of "Internet" but I guess there's data behind it, as close to a tie as that might be 😅 .

Copy link
Member

@jsnajdr jsnajdr Sep 1, 2025

Choose a reason for hiding this comment

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

On the other hand, Core uses lower-case "internet". Although the sample is very small, there are just two messages that use the word:

  • "Keeping your site updated is important for security. It also makes the internet a safer place for you and your readers."
  • "The internet address of your network will be %s." (when configuring the network feature)

Capital I is used only when talking about Internet Explorer.

Copy link
Contributor Author

@mcsf mcsf Sep 1, 2025

Choose a reason for hiding this comment

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

I also checked the WP Glossary, and both are found throughout. :) Capitalised is how I personally write it, but I can go either way here. @jsnajdr, are you invested in the non-capitalised form?

Copy link
Member

@jsnajdr jsnajdr Sep 1, 2025

Choose a reason for hiding this comment

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

I'm not invested at all, was just curious about which convention is actually used in WordPress. 🙂 The Gutenberg repo also leans heavily towards lowercase, although it's all comments and docs, there's zero usage in code.

),
};
}

// Hard to diagnose further due to how Window.fetch reports errors.
throw {
code: 'fetch_error',
message: __( 'You are probably offline.' ),
message: __(
'Could not get a valid response from the server.'
),
};
}
);
Expand Down
25 changes: 21 additions & 4 deletions packages/editor/src/store/utils/notice-builder.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -89,17 +89,34 @@ export function getNotificationArgumentsForSaveFail( data ) {

const publishStatus = [ 'publish', 'private', 'future' ];
const isPublished = publishStatus.indexOf( post.status ) !== -1;
// If the post was being published, we show the corresponding publish error message
// Unless we publish an "updating failed" message.

if ( error.code === 'offline_error' ) {
const messages = {
publish: __( 'Publishing failed because you were offline.' ),
private: __( 'Publishing failed because you were offline.' ),
future: __( 'Scheduling failed because you were offline.' ),
default: __( 'Updating failed because you were offline.' ),
Comment on lines +95 to +98
Copy link
Member

@tyxla tyxla Sep 1, 2025

Choose a reason for hiding this comment

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

I wonder if we can move "You were offline." to a separate string that we concat with the existing "publishing/updating/scheduling failed" string. That way translators will have less to translate. WDYT?

Copy link
Member

@Mamaduka Mamaduka Sep 1, 2025

Choose a reason for hiding this comment

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

Concatenating two separately translated strings isn't recommended, as far as I can remember. Based on the language, you may receive an odd complete translation.

Copy link
Member

@tyxla tyxla Sep 1, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

I agree in general if we were separating a single sentence, but perhaps it should be fine if they are 2 separate sentences?

Copy link
Member

@Mamaduka Mamaduka Sep 1, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

@swissspidy or @SergeyBiryukov might have a more definitive answer. I don't remember any exceptions to the rule, but they usually have a couple 😄

Copy link
Contributor Author

@mcsf mcsf Sep 1, 2025

Choose a reason for hiding this comment

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

Exactly, I intentionally inlined all possible sentences, the same way that we need to define all possible CPT labels one by one.

Copy link
Member

@tyxla tyxla Sep 1, 2025

Choose a reason for hiding this comment

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

Thanks. If there's a rule, sacrificing some extra work to add a few extra strings sounds like it's worth it. I'm just mindful about the translation efforts since every core string change requires hundreds of people's attention.

Copy link
Member

@swissspidy swissspidy Sep 1, 2025

Choose a reason for hiding this comment

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

If you wanna concatenate two sentences "Publishing failed." and "You are offline", that should be OK. But it's only 4 extra strings here which is also fine.

Alternatives would be to have a generic text only (e.g. "Action failed because you are probably offline"), or WP adding support for custom labels per post status.

};

const noticeMessage =
! isPublished && edits.status in messages
? messages[ edits.status ]
: messages.default;

return [ noticeMessage, { id: 'editor-save' } ];
}

const messages = {
publish: __( 'Publishing failed.' ),
private: __( 'Publishing failed.' ),
future: __( 'Scheduling failed.' ),
default: __( 'Updating failed.' ),
};

let noticeMessage =
! isPublished && publishStatus.indexOf( edits.status ) !== -1
! isPublished && edits.status in messages
? messages[ edits.status ]
: __( 'Updating failed.' );
: messages.default;

// Check if message string contains HTML. Notice text is currently only
// supported as plaintext, and stripping the tags may muddle the meaning.
Expand Down
Loading