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

[RNMobile] update mobile to not use ListEdit #17070

Merged
merged 3 commits into from Aug 20, 2019

Conversation

@mchowning
Copy link
Contributor

commented Aug 16, 2019

related gutenberg PR

Description

This update is in response to changes made on the web side in #15113
that resulted in a bundling error on mobile due to a missing module. Because the web side now uses a PanelBody component that mobile does not have in part of the list implementation that is shared with mobile, we needed to separate that out on the mobile side.

Sharing Logic Between Web and Mobile

I went with the straightforward and safe approach of just creating a separate list/edit.native.js file that is a copy of list/edit.js but without the components that mobile does not use/have. This seemed like the best approach because it is both quick and safe since it doesn't change any code that the web relies on. It also seems to be the approach I've seen used elsewhere. With that said, I don't really like this as a general approach because not only do now have most of list/edit.js duplicated in two files, but list/edit.js and listedit.native.js are now going to almost inevitably drift apart, which will likely lead to further problems like the one that led to this PR. Long story short, I'm trying to think about what might be a better long-term approach, so please don't hesitate to suggest another approach that might be better.

The only real substantive change in list/edit.native.js from list/edit.js is the removal of the "Start Value" and "Reverse" controls that are used on the web. For example, I left in the keyboard shortcut components from web even though we're not using them on mobile in order to (1) keep the native implementation as similar to web as possible; and (2) because I know we want to add keyboard shortcuts on mobile eventually.

I pushed a new commit moving away from ^that approach. Instead, I am now separating the logic by extracting the web-only part of the component to a separate additional-settings.js file/component. Then for mobile I just made a component function in additional-settings.native.js that always returns null. I prefer this because it avoids the duplication in the separate edit.native.js file and also makes it clear where mobile and web are differing.

A similar approach would be to just give mobile an empty component for all of the components used on web but not on mobile (fedbdc8 shows what this PR would look like with that approach). The disadvantage of this imo is that it makes it less obvious to determine after-the-fact where the behavior of mobile and web differs as compared to having the single additional-settings component like this PR has. The advantage of this approach, however, is that it requires no changes to the web-side's js files.

How has this been tested?

Mobile

Without the changes in this PR, attempting to bundle gutenberg's master branch for mobile should result in a bundling error. With the changes in this PR there is no error and no problems or changes in behavior with respect to:

  • Loading web-created post with a list block on mobile
  • Viewing mobile-created post with a list block on web
  • Adjusting indentation in a list block on mobile
  • Adjusting bulllet style (ul/ol) on mobile

Web

Tested that this PR causes no changes on the web side. In particular, verified that the changes added in #15113 work as they did before with ordered lists:

  • Modifying the "Start Value" setting
  • Modifying the "Reverse List" setting

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.
This update is in response to changes made on the web side in #15113
that were causing a crash on mobile.
@mchowning

This comment has been minimized.

Copy link
Contributor Author

commented Aug 16, 2019

👋 @ellatrix . You may want to give this PR a look since it relates to some recent changes in #15113.

Copy link
Contributor

left a comment

I do prefer this approach too.
On the second proposal, having empty components implementation might be problematic when we do implement those component for other purposes.

This solves the building error and seems to work fine on Mobile 👍

I wonder what would happen when we load a list block using these new props on mobile.
Tried running gutenberg web locally from latest master but I didn't see the new options on Ordered List.

ToggleControl,
} from '@wordpress/components';

const AdditionalSettings = ( { setAttributes, ordered, reversed, start } ) =>

This comment has been minimized.

Copy link
@ellatrix

ellatrix Aug 19, 2019

Member

Just a suggestion: perhaps call it OrderedListSettings and move the ordered check?

This comment has been minimized.

Copy link
@mchowning

mchowning Aug 19, 2019

Author Contributor

I like that much better. Thanks for the suggestion!

@ellatrix

This comment has been minimized.

Copy link
Member

commented Aug 19, 2019

This is fine with me if it solves the error. :)

@mchowning mchowning force-pushed the rnmobile/fix_ListEdit_on_mobile branch from 5618f01 to 0f74d0a Aug 19, 2019
@mchowning

This comment has been minimized.

Copy link
Contributor Author

commented Aug 19, 2019

👋 @etoledom Thanks for the review!

I wonder what would happen when we load a list block using these new props on mobile.

I updated initial-html.js to include this block (copied from html mode on a local server running the web editor from this branch), and I didn't observe any problems on mobile:

<!-- wp:list {"ordered":true,"start":10,"reversed":true} -->
<ol reversed start="10"><li>First</li><li>Second<ol><li>Third Indented</li></ol></li><li>Fourth</li></ol>
<!-- /wp:list -->

Tried running gutenberg web locally from latest master but I didn't see the new options on Ordered List.

Well that is concerning! I just double-checked this and everything seems to be working fine for me when I run a local dev server. I checked that I was actually observing the changes from my branch on my local dev server by observing that running the server off of my branch shows the new options if no changes are made, but if I made changes that broke the new options then those options did not appear for me (in other words, my breaking the feature during development was most-definitely just a part of my carefully-considered and oh-so-thorough testing strategy, it definitely wasn't that I just messed something up 😉).

If you're still not seeing the additional options on web, let's sync up and figure out what we're doing differently.

Copy link
Contributor

left a comment

Thank you for the update, great job 🎉

I managed to run locally gutenberg-web successfully now, and I noticed something that I thought might happen:

I updated initial-html.js to include this block (copied from html mode on a local server running the web editor from this branch), and I didn't observe any problems on mobile

So, gutenberg-mobile loads the block properly, except that it doesn't take into account the new parameters. This is expected since we haven't implemented that functionality on Aztec.

The problem is that if we modify the list block in any way, the new settings will be lost and will override what was done on web.

This is probably difficult to fix and will have us implementing that functionality, so I think the best would be to merge this and solve the build error, and to create a ticket with this issue to keep it in mind.

Thank you again @mchowning for tacking this!

@mchowning

This comment has been minimized.

Copy link
Contributor Author

commented Aug 20, 2019

The problem is that if we modify the list block in any way, the new settings will be lost and will override what was done on web.

This is probably difficult to fix and will have us implementing that functionality, so I think the best would be to merge this and solve the build error, and to create a ticket with this issue to keep it in mind.

Good catch @etoledom ! I'll make the ticket.

UPDATE: Created gutenberg-mobile#1304

@mchowning mchowning merged commit db70326 into master Aug 20, 2019
4 checks passed
4 checks passed
Filter opened
Details
Filter opened
Details
Milestone It
Details
Travis CI - Pull Request Build Passed
Details
@mchowning mchowning deleted the rnmobile/fix_ListEdit_on_mobile branch Aug 20, 2019
@jorgefilipecosta jorgefilipecosta added this to the Gutenberg 6.4 milestone Aug 26, 2019
donmhico added a commit to donmhico/gutenberg that referenced this pull request Aug 27, 2019
* [RNMobile] update mobile to not use ListEdit

This update is in response to changes made on the web side in WordPress#15113
that were causing a crash on mobile.

* Extract list block logic not shared with mobile to separate component

* Improve name of AdditionalSettings component
gziolo added a commit that referenced this pull request Aug 29, 2019
* [RNMobile] update mobile to not use ListEdit

This update is in response to changes made on the web side in #15113
that were causing a crash on mobile.

* Extract list block logic not shared with mobile to separate component

* Improve name of AdditionalSettings component
gziolo added a commit that referenced this pull request Aug 29, 2019
* [RNMobile] update mobile to not use ListEdit

This update is in response to changes made on the web side in #15113
that were causing a crash on mobile.

* Extract list block logic not shared with mobile to separate component

* Improve name of AdditionalSettings component
dd32 pushed a commit to dd32/gutenberg that referenced this pull request Sep 27, 2019
* [RNMobile] update mobile to not use ListEdit

This update is in response to changes made on the web side in WordPress#15113
that were causing a crash on mobile.

* Extract list block logic not shared with mobile to separate component

* Improve name of AdditionalSettings component
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.