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

Port nextpage block to the ReactNative mobile app #11192

Merged
merged 7 commits into from Oct 30, 2018

Conversation

pinarol
Copy link
Contributor

@pinarol pinarol commented Oct 29, 2018

Description

This PR contains the minimum set of required changes for ReactNative mobile app be able to use the nextpage block. Changes can be tested using this PR.

How has this been tested?

  • Make sure the (web) GB build and webapp work normally
  • Make sure the GB mobile app works normally and nextpage block is available for adding.

Types of changes

  • Nextpage block is added to the list of registered blocks for mobile app
  • Edit function is extracted from index.js and put into separate files for mobile(edit.native.js) and web(edit.js)
  • New .scss is added for mobile: editor.native.scss

Checklist:

  • My code is tested.
  • My code follows the WordPress code style.
  • My code has proper inline documentation.

* Edit function is extracted from index.js and put into separate files for mobile and web
* New scss is added for mobile
@pinarol pinarol added the Mobile Web Viewport sizes for mobile and tablet devices label Oct 29, 2018
@pinarol pinarol self-assigned this Oct 29, 2018
@pinarol pinarol requested review from koke, Tug, hypest, diegoreymendez and etoledom and removed request for diegoreymendez and etoledom October 29, 2018 09:46
@pinarol pinarol added Mobile App - i.e. Android or iOS Native mobile impl of the block editor. (Note: used in scripts, ping mobile folks to change) and removed Mobile Web Viewport sizes for mobile and tablet devices labels Oct 30, 2018
@@ -21,6 +21,7 @@
"build-module"
],
"main": "build/index.js",
"react-native": "src/index",
Copy link
Member

Choose a reason for hiding this comment

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

👍 we missed that, sorry.


.block-library-nextpage__container {
align-items: center;
padding-left: 4;
Copy link
Member

Choose a reason for hiding this comment

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

Does RN version of sass support shortcuts present in CSS like padding: 4px?

@hypest this is confusing to have something which looks like CSS but isn't. Maybe we should revisit this approach and start using JS files with style objects instead?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

padding: 4px worked! 👍

Copy link
Contributor

Choose a reason for hiding this comment

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

Does RN version of sass support shortcuts present in CSS like padding: 4px

I think it does, one just needs to provide all 4 or more of the individual fields. Example: padding: 4px 4px 4px 4px.

@hypest this is confusing to have something which looks like CSS but isn't. Maybe we should revisit this approach and start using JS files with style objects instead?

I think we haven't really put any new effort on the styling side of things so, let's defer this convo for a bit. I think we will do style related work later in the year.

Copy link
Member

@gziolo gziolo left a comment

Choose a reason for hiding this comment

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

This PR looks good. I don't see any blockers. I left one comment where you can shorten the code.

In particutlar const value = customText !== undefined ? customText : defaultText; is something that is very hard to read. You usually will see it expressed as const value = customText || defaultText;

padding-right: 4;
padding-top: 4;
padding-bottom: 4;
padding: 4px 4px 4px 4px;
Copy link
Member

Choose a reason for hiding this comment

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

In real CSS it can be even shorter: padding: 4px;

@pinarol pinarol merged commit 9d46596 into master Oct 30, 2018
@youknowriad youknowriad added this to the 4.2 milestone Oct 30, 2018
@koke
Copy link
Contributor

koke commented Oct 30, 2018

🎉 congrats on your first merge!

@hypest hypest deleted the rnmobile/port-nextpage-block branch October 30, 2018 16:51
@gziolo
Copy link
Member

gziolo commented Oct 30, 2018

Congrats 💯

antpb pushed a commit to antpb/gutenberg that referenced this pull request Nov 5, 2018
* Port next page block to native app

* Edit function is extracted from index.js and put into separate files for mobile and web
* New scss is added for mobile

* Fix lint issues

* Fix failing unittest

* Add react-native path to notices package.json

* Make some code review fixes
ghost pushed a commit to OnTheGoSystems/gutenberg that referenced this pull request Nov 22, 2018
* Port next page block to native app

* Edit function is extracted from index.js and put into separate files for mobile and web
* New scss is added for mobile

* Fix lint issues

* Fix failing unittest

* Add react-native path to notices package.json

* Make some code review fixes
ghost pushed a commit to OnTheGoSystems/gutenberg that referenced this pull request Jan 4, 2019
* Port next page block to native app

* Edit function is extracted from index.js and put into separate files for mobile and web
* New scss is added for mobile

* Fix lint issues

* Fix failing unittest

* Add react-native path to notices package.json

* Make some code review fixes
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