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

[Modal Layout Picker] "Create Page" button functionality #12943

Merged
merged 22 commits into from
Sep 29, 2020

Conversation

antonis
Copy link
Contributor

@antonis antonis commented Sep 11, 2020

Fixes: wordpress-mobile/gutenberg-mobile#2446

Related PRs:

Description

This PR adds the functionality to the "Create Page" button

To test:

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

🗒️ The layouts that are provided right now do contain a lot of unsupported blocks. These will be filtered out by a later implementation.

From My Site

  1. On the My Site Page
  2. Click the FAB icon
  3. Select Site Page
  4. Choose a Layout
  5. Select Create Page

Expect to see the Editor loaded with the contents of your selected block

From Pages

  1. On the My Site Page
  2. Open the Page list
  3. Select the ➕ icon
  4. Choose a Layout
  5. Select Create Page

Expect to see the Editor loaded with the contents of your selected block

Note: This PR includes some minor design fixes

Screenshots:

Supported Unsupported
device-2020-09-24-000345 device-2020-09-24-000248

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

peril-wordpress-mobile bot commented Sep 11, 2020

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

@antonis antonis self-assigned this Sep 11, 2020
@peril-wordpress-mobile
Copy link

peril-wordpress-mobile bot commented Sep 11, 2020

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

@antonis antonis marked this pull request as ready for review September 22, 2020 13:52
@antonis antonis changed the title [Modal Layout Picker] "Create Page" button functionality [**DO NOT MERGE**] [Modal Layout Picker] "Create Page" button functionality Sep 22, 2020
@antonis antonis added this to the 15.9 milestone Sep 22, 2020
@antonis antonis force-pushed the issue/2446-MLP_CreatePage branch from ada5da6 to 55ed133 Compare September 22, 2020 14:19
Base automatically changed from issue/2443-MLP_WPCOM to develop September 23, 2020 10:53
@antonis antonis requested a review from malinajirka September 23, 2020 11:05
@malinajirka
Copy link
Contributor

I can review this PR tomorrow unless someone beats me to it.

@antonis antonis force-pushed the issue/2446-MLP_CreatePage branch from 3286d6e to 7db8801 Compare September 24, 2020 13:41
@malinajirka malinajirka self-assigned this Sep 25, 2020
@antonis antonis added the Part of a WIP Feature This label is used to disable milestone checks for PRs that are not against `develop` or `release`. label Sep 25, 2020
Copy link
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.

Thanks @antonis! The code changes look good overall. However, I've found some bugs, but I'm not sure if they are related to this PR. Either way, I can't test it properly.

  1. The "unsupported blog" appears only after I rotate the device. There is a bunch of dialogs stacked on top of each other.
    P.S. Ignore the red logs :)
    dialogs

2. When I select a layout -> make a change in the editor -> rotate the device -> 💥

mqt_native_modules
    Process: org.wordpress.android.prealpha, PID: 2963
    com.facebook.react.common.JavascriptException: TypeError: undefined is not a function, js engine: hermes, stack:
    anonymous@1899:3372
    apply@-1
    value@75:1280
    apply@-1
    value@55:3685
    anonymous@55:841
    value@55:2939
    value@55:813
        at com.facebook.react.modules.core.ExceptionsManagerModule.reportException(ExceptionsManagerModule.java:71)
        at java.lang.reflect.Method.invoke(Native Method)
        at com.facebook.react.bridge.JavaMethodWrapper.invoke(JavaMethodWrapper.java:371)
        at com.facebook.react.bridge.JavaModuleWrapper.invoke(JavaModuleWrapper.java:150)
        at com.facebook.react.bridge.queue.NativeRunnable.run(Native Method)
        at android.os.Handler.handleCallback(Handler.java:790)
        at android.os.Handler.dispatchMessage(Handler.java:99)

@antonis
Copy link
Contributor Author

antonis commented Sep 25, 2020

Hello @malinajirka 👋 ,

thank you for reviewing this today. I really appreciate it 🙇

Thanks @antonis! The code changes look good overall. However, I've found some bugs, but I'm not sure if they are related to this PR. Either way, I can't test it properly.

Both bugs are reproduced with layouts that contain unsupported blocks. Those will be filtered out by the next PR and are out of scope for testing in my understanding (cc @chipsnyder ).
Sorry for not being specific about this. Including the unsupported blocks screenshot made it more confusing 🤷

I tried to reproduce bug (2) with a supported layout but couldn't reproduce the crash. Let me know if that's not the case.
Check with a supported layout if possible (e.g. the first in the Home pages category).

@antonis antonis requested a review from malinajirka September 25, 2020 16:51
@chipsnyder
Copy link
Contributor

Both bugs are reproduced with layouts that contain unsupported blocks. Those will be filtered out by a later implementation and are out of scope for testing in my understanding (cc @chipsnyder ).

That's accurate the unsupported blocks will be filtered out later. Thanks for calling these issues out though they both appear to issues with the Gutenberg editor as a whole. I'll try to reproduce these and open an issue in Gutenberg for them

Copy link
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.

Ohh I see. When you mentioned that we'll filter out the layouts with unsupported blogs I thought I should just ignore the unsupported blogs in the editor - not that I shouldn't use them for testing. Either way, I re-tested the app and the content is added as expected. ;) Good job! (btw issue 2 happens even with a supported layout cc @chipsnyder - let me know if I should report it or you'll take care of it)

@malinajirka malinajirka merged commit 1671da4 into develop Sep 29, 2020
@malinajirka malinajirka deleted the issue/2446-MLP_CreatePage branch September 29, 2020 06:10
@chipsnyder
Copy link
Contributor

@malinajirka @antonis

I went ahead and created a couple of issues for the problems @malinajirka pointed out, thanks again for bringing these up :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Part of a WIP Feature This label is used to disable milestone checks for PRs that are not against `develop` or `release`. [Type] Enhancement
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants