Skip to content

[Modal Layout Picker] "Preview" button functionality#13022

Merged
antonis merged 20 commits intodevelopfrom
issue/2452-Preview
Oct 16, 2020
Merged

[Modal Layout Picker] "Preview" button functionality#13022
antonis merged 20 commits intodevelopfrom
issue/2452-Preview

Conversation

@antonis
Copy link
Copy Markdown
Contributor

@antonis antonis commented Sep 28, 2020 •
edited
Loading

Fixes: wordpress-mobile/gutenberg-mobile#2452

Related PRs:

  1. [Mobile] Adds editorMode initial property WordPress/gutenberg#25661
  2. [RNMobile] Update Preview handling for better Android support WordPress/gutenberg#25930
  • gutenberg-mobile:
  1. [Modal Layout Picker] Adds Editor Mode for the "Preview" button functionality gutenberg-mobile#2674
  2. Update Preview handling for better Android support gutenberg-mobile#2696
  • WordPress-FluxC-Android
  1. Parcelizes FETCH_BLOCK_LAYOUTS response WordPress-FluxC-Android#1715

To test:

Layout Picker should show when creating a new page from My Site or Site Pages the Modal Layout Picker appears.

  • Start by navigating to the Modal Layout Picker

Preview a layout

  1. Select a layout from the list
  2. Select Preview
  • Expect to see a read-only editor load that shows a preview of the selected layout.

Preview a layout, then a different one

  1. Select a layout from the list
  2. Select Preview
  • Expect to see a read-only editor load that shows a preview of the selected layout.
  1. Select Back
  2. Select a different layout from the list
  3. Select Preview
  • Expect to see a read-only editor load that shows a preview of the newly selected layout.

Preview a layout, then cancel

  1. Select a layout from the list
  2. Select Preview
  • Expect to see a read-only editor load that shows a preview of the selected layout.
  1. Select Back
  • Or close the modal by swiping down from the top
  1. Close the modal via the ✖️

Preview a layout, then create a page

  1. Select a layout from the list
  2. Select Preview
  • Expect to see a read-only editor load that shows a preview of the selected layout.
  1. Select Create Page
  • Expect to see the page editor load with the selected layout content populated.
  • Expect to see the page editor load with a title that relates to the content. Example a layout from the "About Pages" should have a title like "About"

Spot check for regressions

Open an existing page or blog, expect the content to be loaded and for normal editing to take place.

Spot check Modal Layout Picker Disabled

  1. Disable Modal Layout Picker
  2. Create a new page
  • Expect to see the existing Starter Page Template Picker
  1. Select an option
  • Expect to see the Preview with "Close" and "Apply" as actions
  1. Try Each option, expect it to behave as it previously did.

Screenshots

NOTES

I tried another approach by creating a separate Gutenberg Editor just for the preview functionality (this is the approach on iOS). This proved quite complex and the amount of code that needed to be duplicated (and maintained) made it a worst approach in my view.
Thus the approach followed in this PR is passing a isPreview flag and making the less changes possible to achieve the desired result.

PR submission checklist:

  • I have considered adding unit tests where possible.
  • I have considered adding accessibility improvements for my changes.
  • I have considered if this change warrants user-facing release notes and have added them to RELEASE-NOTES.txt if necessary.

@peril-wordpress-mobile
Copy link
Copy Markdown

peril-wordpress-mobile bot commented Sep 28, 2020 •
edited
Loading

You can trigger optional UI/connected tests for these changes by visiting CircleCI here.

This was referenced Sep 28, 2020
Merged
Merged
@peril-wordpress-mobile
Copy link
Copy Markdown

peril-wordpress-mobile bot commented Sep 28, 2020 •
edited
Loading

You can test the changes on this Pull Request by downloading the APK here.

antonis added 3 commits October 6, 2020 10:54
@antonis
Merge branch 'issue/2444-MLP_SupportedBlocks' into issue/2452-Preview
e22413e 
# Conflicts:
#	WordPress/src/main/java/org/wordpress/android/ui/posts/EditPostActivity.java
#	libs/editor/WordPressEditor/src/main/java/org/wordpress/android/editor/gutenberg/GutenbergPropsBuilder.kt
#	libs/gutenberg-mobile
@antonis
Sets Activity result in preview mode
84e4909
@antonis
Discarts page layout preview
5852c21
@antonis antonis force-pushed the issue/2452-Preview branch from ace2a61 to 5852c21 Compare October 7, 2020 07:35
@antonis antonis force-pushed the issue/2452-Preview branch from 4a6784e to db36fd0 Compare October 7, 2020 12:55
@antonis antonis self-assigned this Oct 7, 2020
@antonis antonis mentioned this pull request Oct 7, 2020
Merged
3 tasks
@antonis antonis changed the title [Modal Layout Picker] "Preview" button functionality [DO NOT MERGE] [Modal Layout Picker] "Preview" button functionality Oct 8, 2020
This was referenced Oct 8, 2020
Merged
Merged
@antonis antonis marked this pull request as ready for review October 8, 2020 06:50
@antonis
Merge branch 'issue/2444-MLP_SupportedBlocks' into issue/2452-Preview
ca668c0 
# Conflicts:
#	WordPress/src/test/java/org/wordpress/android/viewmodel/mlp/ModalLayoutPickerViewModelTest.kt
@malinajirka malinajirka self-assigned this Oct 12, 2020
malinajirka
malinajirka reviewed Oct 12, 2020
View reviewed changes
Copy link
Copy Markdown
Contributor

@malinajirka malinajirka left a comment

Choose a reason for hiding this comment

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

Great job @antonis! The code looks good.

I'm not sure if this was discussed or not, so I'm better sharing it. We used to have support for local previews in the past. They were removed in this ticket. Mostly because the local preview never looks the same as the actual site - it's missing css. Since we removed local previews I'm wondering if it make sense to re-introduce them for page layouts. AFAIK they have exactly the same issues - I tested a few layouts and encountered differences between the editor view and preview in a webview. If this was already discussed, please ignore this comment.

Some issues I noticed

  1. When the app crashes/the user restarts the app while in preview, a draft is created. I'm not sure it's an issue, just mentioning it so we are aware of it.

  2. Title is always empty - I tested 6 layouts on a self-hosted and .com sites.
    missing-title

  3. "Don't keep activities" in developer settings results in an empty layout chooser.
    dontkeepactivities

@antonis
Copy link
Copy Markdown
Contributor Author

antonis commented Oct 12, 2020 •
edited
Loading

Hello @malinajirka 👋 ,

thank you for taking the time to review this 🙇

I'm not sure if this was discussed or not, so I'm better sharing it. We used to have support for local previews in the past. They were removed in this ticket. Mostly because the local preview never looks the same as the actual site - it's missing css. Since we removed local previews I'm wondering if it make sense to re-introduce them for page layouts. AFAIK they have exactly the same issues - I tested a few layouts and encountered differences between the editor view and preview in a webview. If this was already discussed, please ignore this comment.

Tbh, I was not aware that local previews were intentionally removed in the past. In my understanding the limitations of the preview functionality were considered during design and there will be a separate project in the future to expand this preview to provide a more WYSIWYG experience. (cc @chipsnyder )

  1. When the app crashes/the user restarts the app while in preview, a draft is created. I'm not sure it's an issue, just mentioning it so we are aware of it.

I will investigate this further.

  1. Title is always empty - I tested 6 layouts on a self-hosted and .com sites.

I fixed this as part of a next pr with 1c3f941

Screenshot
  1. "Don't keep activities" in developer settings results in an empty layout chooser.

I will investigate this further.

@antonis antonis mentioned this pull request Oct 12, 2020
Merged
@antonis
Copy link
Copy Markdown
Contributor Author

antonis commented Oct 12, 2020 •
edited
Loading

  1. When the app crashes/the user restarts the app while in preview, a draft is created. I'm not sure it's an issue, just mentioning it so we are aware of it.

Tried to handle this in the onDestroy method of EditPostActivity unsuccessfully. I'm not sure if I can prevent this from happening without major changes in the editor.

  1. "Don't keep activities" in developer settings results in an empty layout chooser.

Fixed with 32e255f and 2b24ee1 (depends on wordpress-mobile/WordPress-FluxC-Android#1715)

@chipsnyder
Copy link
Copy Markdown
Contributor

chipsnyder commented Oct 12, 2020

Tbh, I was not aware that local previews were intentionally removed in the past. In my understanding the limitations of the preview functionality were considered during design and there will be a separate project in the future to expand this preview to provide a more WYSIWYG experience. (cc @chipsnyder )

Thanks for sharing that ticket about Previews @malinajirka. When planning this project we discussed more of WYSIWYG experience but it was determined that reusing the Preview that is currently shown as part of the Gutenberg preview would be sufficient for this project. (internal reference: pb3aDo-ny) but we are discussing with @iamthomasbishop about what the next phase of the preview will look like.

Base automatically changed from issue/2444-MLP_SupportedBlocks to develop October 13, 2020 06:23
@antonis antonis added this to the 16.0 milestone Oct 13, 2020
@antonis antonis requested a review from malinajirka October 13, 2020 06:34
malinajirka
malinajirka approved these changes Oct 16, 2020
View reviewed changes
Copy link
Copy Markdown
Contributor

@malinajirka malinajirka left a comment

Choose a reason for hiding this comment

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

LGTM, thanks @antonis!

@antonis antonis merged commit bf31123 into develop Oct 16, 2020
@antonis antonis deleted the issue/2452-Preview branch October 16, 2020 07:22
This was referenced Nov 11, 2020
Closed
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@malinajirka malinajirka malinajirka approved these changes

@ashiagr ashiagr Awaiting requested review from ashiagr

@chipsnyder chipsnyder Awaiting requested review from chipsnyder

@develric develric Awaiting requested review from develric

Assignees

@antonis antonis

@malinajirka malinajirka

Labels

[Type] Enhancement

Projects

None yet

Milestone

16.0

Development

Successfully merging this pull request may close these issues.

3 participants

@antonis @chipsnyder @malinajirka