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

Add the react-native-hr library to GB mobile #205

Merged
merged 9 commits into from
Nov 1, 2018

Conversation

pinarol
Copy link
Contributor

@pinarol pinarol commented Oct 29, 2018

This is a simple PR that adds the react-native-hr library to GB mobile. It will be used by the nextpage block. Usage can be found in this PR.

I've updated the submodule ref to point to the gutenberg branch that uses the new Hr element. So you can open the demo app and verify that there is a text-surrounding line in the Page break element which is brought us by this new Hr element:

screen shot 2018-10-31 at 15 25 22

@pinarol pinarol changed the title Feature/port nextpage block dds the react-native-hr library to GB Feature/port nextpage block Oct 29, 2018
@pinarol pinarol changed the title dds the react-native-hr library to GB Feature/port nextpage block Add the react-native-hr library to GB mobile Oct 29, 2018
@hypest
Copy link
Contributor

hypest commented Oct 29, 2018

👋 @pinarol , is there perhaps a GB-web side branch or PR to link with in this PR? I don't see GB-web ref being updated or a PR referenced so, not sure how to test this one? Thanks!

@hypest
Copy link
Contributor

hypest commented Oct 29, 2018

This PR is a followup from #209 so, let's wait for #209 before moving on with this one. Is that OK @pinarol ? Thanks!

@pinarol
Copy link
Contributor Author

pinarol commented Oct 29, 2018

right let's wait for #209 firstly

This is for being able to test the new Hr element
@pinarol
Copy link
Contributor Author

pinarol commented Oct 31, 2018

this is ready to review @hypest I updated the GB-web ref to include the example usage

@etoledom
Copy link
Contributor

Hey @pinarol - This is looking great! 🎉

I couldn't find the Gutenberg side PR, though.
Can we have a review on that side before merging this one?

Thank you!

@pinarol
Copy link
Contributor Author

pinarol commented Nov 1, 2018

Hi @etoledom here I opened it, I didn't open it before because it won't compile without this change so this PR needs to be merged first, but yes it'd be better if reviewers at least see the related change. 👍

@pinarol
Copy link
Contributor Author

pinarol commented Nov 1, 2018

@hypest I thought the gutenberg PR will not compile without the new lib introduced with this PR but it looks like tests have passed. So should I merge this one first?

@hypest
Copy link
Contributor

hypest commented Nov 1, 2018

I notice that the Gutenberg side PR only touches .native files and as of WordPress/gutenberg#11318 Travis will happily say everything's fine. So, the only way to test if the GB side PR will break the mobile tests is to try it manually (try using that GB branch against mobile master that is).

Now, we do actually know that the import on the GB side will break against current mobile master in some way so, let's review and give the green light here and then quickly merge the GB side, update this PR's ref and merge it too. The total disruption to mobile master will be small. WDYT?

@pinarol
Copy link
Contributor Author

pinarol commented Nov 1, 2018

@hypest I agree, it looks like it won't be a problem as soon as I'm quick between PR merges. So once PR reviews are done I can just merge GB PR and then update this PR's ref and merge it too 👍

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.

It looks great and all tests are passing (so far 😅)
@pinarol - All yours to do the merge magic!

@pinarol
Copy link
Contributor Author

pinarol commented Nov 1, 2018

We know that there's a known issue with travis checks currently and we are working on it, but we decided to merge this immediately in order not to cause compile errors on local working environments since a new library is introduced with this PR and it is already imported from the gutenberg submodule.

@pinarol pinarol merged commit cac06dc into master Nov 1, 2018
@pinarol pinarol deleted the feature/port-nextpage-block branch November 1, 2018 11:28
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants