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

Ensure that the autosave happens on empty posts in the native apps. #23233

Merged

Conversation

SergioEstevao
Copy link
Contributor

@SergioEstevao SergioEstevao commented Jun 17, 2020

Description

On the native apps, the publish button is managed by native code on
the apps (non Gutenberg/JS code). In order to update the status of the
publish button correctly, we need to autosave even when there is no
content, in order to send that information across the native code.

How has this been tested?

This can be tested using this WP-iOS PR: wordpress-mobile/WordPress-iOS#14329

Screenshots

Types of changes

Checklist:

  • My code is tested.
  • My code follows the WordPress code style.
  • My code follows the accessibility standards.
  • My code has proper inline documentation.
  • I've included developer documentation if appropriate.
  • I've updated all React Native files affected by any refactorings/renamings in this PR.

On the native apps, the publish button is managed by native code on
the apps (non Gutenberg/JS code). In order to update the status of the
publish button correctly we need to autosave even when there is no
content, in order to send that information across the native code.
@SergioEstevao SergioEstevao added the Mobile App - i.e. Android or iOS Native mobile impl of the block editor. (Note: used in scripts, ping mobile folks to change) label Jun 17, 2020
@github-actions
Copy link

github-actions bot commented Jun 17, 2020

Size Change: +13 B (0%)

Total Size: 1.15 MB

Filename Size Change
build/editor/index.min.js 45.2 kB +13 B (0%)
ℹ️ View Unchanged
Filename Size Change
build/a11y/index.min.js 1.14 kB 0 B
build/annotations/index.min.js 3.67 kB 0 B
build/api-fetch/index.min.js 3.43 kB 0 B
build/autop/index.min.js 2.82 kB 0 B
build/blob/index.min.js 620 B 0 B
build/block-directory/index.min.js 7.63 kB 0 B
build/block-directory/style-rtl.css 944 B 0 B
build/block-directory/style.css 945 B 0 B
build/block-editor/index.min.js 125 kB 0 B
build/block-editor/style-rtl.css 10.8 kB 0 B
build/block-editor/style.css 10.8 kB 0 B
build/block-library/editor-rtl.css 7.58 kB 0 B
build/block-library/editor.css 7.57 kB 0 B
build/block-library/index.min.js 132 kB 0 B
build/block-library/style-rtl.css 7.77 kB 0 B
build/block-library/style.css 7.77 kB 0 B
build/block-library/theme-rtl.css 728 B 0 B
build/block-library/theme.css 729 B 0 B
build/block-serialization-default-parser/index.min.js 1.88 kB 0 B
build/block-serialization-spec-parser/index.min.js 3.1 kB 0 B
build/blocks/index.min.js 48.3 kB 0 B
build/components/index.min.js 200 kB 0 B
build/components/style-rtl.css 15.6 kB 0 B
build/components/style.css 15.6 kB 0 B
build/compose/index.min.js 9.67 kB 0 B
build/core-data/index.min.js 11.5 kB 0 B
build/data-controls/index.min.js 1.29 kB 0 B
build/data/index.min.js 8.45 kB 0 B
build/date/index.min.js 5.38 kB 0 B
build/deprecated/index.min.js 772 B 0 B
build/dom-ready/index.min.js 568 B 0 B
build/dom/index.min.js 3.23 kB 0 B
build/edit-navigation/index.min.js 10.8 kB 0 B
build/edit-navigation/style-rtl.css 1.08 kB 0 B
build/edit-navigation/style.css 1.08 kB 0 B
build/edit-post/index.min.js 304 kB 0 B
build/edit-post/style-rtl.css 5.61 kB 0 B
build/edit-post/style.css 5.61 kB 0 B
build/edit-site/index.min.js 16.8 kB 0 B
build/edit-site/style-rtl.css 3.06 kB 0 B
build/edit-site/style.css 3.06 kB 0 B
build/edit-widgets/index.min.js 9.35 kB 0 B
build/edit-widgets/style-rtl.css 2.45 kB 0 B
build/edit-widgets/style.css 2.45 kB 0 B
build/editor/editor-styles-rtl.css 537 B 0 B
build/editor/editor-styles.css 539 B 0 B
build/editor/style-rtl.css 3.78 kB 0 B
build/editor/style.css 3.78 kB 0 B
build/element/index.min.js 4.65 kB 0 B
build/escape-html/index.min.js 733 B 0 B
build/format-library/index.min.js 7.72 kB 0 B
build/format-library/style-rtl.css 547 B 0 B
build/format-library/style.css 548 B 0 B
build/hooks/index.min.js 2.13 kB 0 B
build/html-entities/index.min.js 621 B 0 B
build/i18n/index.min.js 3.56 kB 0 B
build/is-shallow-equal/index.min.js 711 B 0 B
build/keyboard-shortcuts/index.min.js 2.51 kB 0 B
build/keycodes/index.min.js 1.94 kB 0 B
build/list-reusable-blocks/index.min.js 3.12 kB 0 B
build/list-reusable-blocks/style-rtl.css 476 B 0 B
build/list-reusable-blocks/style.css 476 B 0 B
build/media-utils/index.min.js 5.32 kB 0 B
build/notices/index.min.js 1.79 kB 0 B
build/nux/index.min.js 3.4 kB 0 B
build/nux/style-rtl.css 671 B 0 B
build/nux/style.css 668 B 0 B
build/plugins/index.min.js 2.56 kB 0 B
build/primitives/index.min.js 1.4 kB 0 B
build/priority-queue/index.min.js 789 B 0 B
build/redux-routine/index.min.js 2.85 kB 0 B
build/rich-text/index.min.js 13.9 kB 0 B
build/server-side-render/index.min.js 2.71 kB 0 B
build/shortcode/index.min.js 1.7 kB 0 B
build/token-list/index.min.js 1.27 kB 0 B
build/url/index.min.js 4.06 kB 0 B
build/viewport/index.min.js 1.85 kB 0 B
build/warning/index.min.js 1.14 kB 0 B
build/wordcount/index.min.js 1.17 kB 0 B

compressed-size-action

Copy link
Contributor

@etoledom etoledom left a comment

Choose a reason for hiding this comment

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

Feels weird having isEditedPostSaveable() returning true` always for native.
If it works better this way for us now we could pass it but this might hit us at some point in the future. It might be good to think about alternatives.

As I understand, this is basically to get the autosave message when the post became empty (user deleted last character). What about we detect when the post gets empty and trigger the autosave message manually?

I'm approving since it works for us now and we can improve on a different PR

@mchowning
Copy link
Contributor

This works for me as expected on iOS. I agree with @etoledom that it feels weird to have isEditablePostSaveable() always return true for native, but I don't have a concrete reason that we shouldn't do that, so I'll approve as well.

Also worth noting that Android doesn't disable the publish button as best I can tell, so this fix does not affect that behavior on Android. Also though, since Android does not save on exiting the editor, this change actually fixes an issue where if you delete the last block and then exit the post on Android, the native side would have never gotten notified about the new empty state, so the "saved" post would be from before the block was deleted. Not a big bug, but it is a bug and this does fix it.

@hypest
Copy link
Contributor

hypest commented Jul 15, 2020

since Android does not save on exiting the editor

Sorry, can you elaborate on that one @mchowning ? I was under the impression that saving does happen upon exit on WPAndroid so, I'm probably missing the circumstances you refer to on this one. Thanks!

@mchowning
Copy link
Contributor

I was under the impression that saving does happen upon exit on WPAndroid so, I'm probably missing the circumstances you refer to on this one.

👋 @hypest ! WPAndroid currently only saves the post via the autosave mechanism; in other words, on exiting the editor the "saved" post is what was saved during the last autosave. For this reason, it is actually possible to type-then-exit-a-post-very-quickly and to lose a small bit of the just-typed text. iOS handles this differently and does save the post on exiting the editor.

Adding save-on-exit on Android is on my todo list, I just haven't had a chance to finish it up yet (ran into some issues where the save seemed to be bizarrely slow and I haven't had a chance to investigate that). The description and comments on my draft PR adding the save-on-exit on Android gives a lot of context around this.

@hypest
Copy link
Contributor

hypest commented Jul 16, 2020

WPAndroid currently only saves the post via the autosave mechanism;

Thanks for the link to the parallel conversation around the fix Matt!
I can be wrong, and just for understanding the history better, did we ever had "save-on-exit" or it's a case missing from the start? I'm under the impression that save-on-exit was there early during the block editor integration to the main app 🤔

@mchowning
Copy link
Contributor

for understanding the history better, did we ever had "save-on-exit" or it's a case missing from the start? I'm under the impression that save-on-exit was there early during the block editor integration to the main app 🤔

I'm not sure, but I bet @malinajirka knows. 🙂

@SergioEstevao SergioEstevao merged commit 3d0a992 into master Jul 17, 2020
@SergioEstevao SergioEstevao deleted the rnmobile/369_ensure_empty_posts_are_not_published branch July 17, 2020 16:43
@github-actions github-actions bot added this to the Gutenberg 8.6 milestone Jul 17, 2020
@malinajirka
Copy link
Contributor

I'm little late to the party, sorry. My notifications for the gutenberg repo were not set-up correctly.

I can be wrong, and just for understanding the history better, did we ever had "save-on-exit" or it's a case missing from the start? I'm under the impression that save-on-exit was there early during the block editor integration to the main app 🤔

I believe we used to have save-on-exit. However, with the introduction of the reactive auto-save approach, this flow became obsolete. Technically there isn't any need to save-on-exit as the reactive auto-save always mirrors the changes from the UI to the model. The fact that we can lose changes from the last 500ms is a bug in the throttle implementation. As per the linked conversation we decided to implement save-on-exit instead of fixing the bug in the "throttle" implementation after all - however, it hasn't been implemented yet.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Mobile App - i.e. Android or iOS Native mobile impl of the block editor. (Note: used in scripts, ping mobile folks to change)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants