-
Notifications
You must be signed in to change notification settings - Fork 798
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
Mobile Contact Info Block #15389
Mobile Contact Info Block #15389
Conversation
Thank you for the great PR description! When this PR is ready for review, please apply the Scheduled Jetpack release: May 5, 2020. |
I know this is still a work in progress, but as a React Native newbie I would have a few questions:
|
@jeherve Thanks for adding the labels and for your comment with helpful links.
So far we have done pretty well sharing linting rules with React JS web files with the gutenberg repository, with only rare exceptions for files with
Despite our ongoing efforts to minmimize gotchas, a few that come to mind include no
Recommendations for Testing/Developing mobile is a work in progress for gutenberg web core blocks maintainers as well, as we currently still require XCode or Android Studio to be installed. I think to start out we can add instructions to the extensions README for testing mobile blocks by providing some context and pointing to the gutenberg-mobile getting started instructions. I started a branch here for adding mobile instructions https://github.com/Automattic/jetpack/compare/rnmobile/add-generic-testing-developing-readme?expand=1. @Tug any extra thoughts about my answers above? |
I have one more, maybe stupid, question. :) We currently do not ship any raw, unbuilt files in Jetpack's stable version. We only ship JavaScript bundles that are built from all the unbuilt files. How will things work with React native files? Will you need to build a new bundle from all the native files, that will be called only from the editor in the mobile app? |
@jeherve That is a good question! My first thought is that the current Jetpack stable release process should not need to include any bundled mobile code ( We currently create JavaScript bundles within tagged versions of our Gutenberg-mobile repo that are then saved as a "Gutenberg Mobile Editor Release" that is then included in the WordPress iOS and Android Apps release process. So our requirements for integrating mobile jetpack code would probably be periodically tagging a snapshot of the jetpack repo as a “Jetpack Blocks Mobile Release” which would correspond to an equivalent "Gutenberg Mobile Editor Release", and "WordPress app release". The jetpack tag would be just to ensure that we can access the raw JavaScript source files snapshot at a specific version if we need to recreate the Jetpack mobile blocks dependency for the respective Gutenberg Mobile and WordPress Mobile app Releases. One natural question that might arise from what I’ve stated above is, why include the mobile versions of blocks within the jetpack repo if they are not being built as part of the Jetpack stable release process you mentioned? The main reason we would still like mobile code in the jetpack repo is to facilitate keeping the mobile and web versions of blocks in synch, with them utilizing many shared “cross platform components”, working towards a future where web and mobile blocks can use either exactly the same code, or code that is reasonable to be developed and maintained simultaneously. It's worth noting that working out the details for integrating jetpack blocks into the WordPress Mobile apps via the .org Gutenberg Editor is still a work in progress, so there may be still more learnings to come regarding our release process, and how we can integrate most smoothly with the existing Jetpack release process. I'll ping @Tug here in case he has anything to add to my answer. |
@jeherve I have one question for you regarding the codeclimate failed scores. Can you let us know what your current conventions are for improving codeclimate scores before merging? I see a fail in CI with 13 issues to fix, and some of the suggestions look like good advice. But do you always address all of the issues, or is there some convention you follow for acceptable scores, or a discussion you can point me to? Forgive me, I'm not familiar with codeclimate, or CI that goes this extra step past automated tests and lint rules. I do like it though. |
That makes sense, thank you for the explanation.
Our CodeClimate configuration is still a work in progress. We need to tweak things some more so it can provide only useful information. Until then, at times it will give useful advice or give you a good alternative point of view on your code that may inspire you to refactor some things. And at times, its advice will be useless. :) |
Code looks good to me for this first iteration 👍 I think there is still a lot to be done, especially in terms of collaborating with the Jetpack team and aligning our designs. @Automattic/jetpack-crew could you maybe have a look at this one? Regarding having a good documentation this is something we want to work on in the next few weeks as we make progress on our ".com/.org code separation" project. Given that Jetpack blocks on mobile are still experimental we're not welcoming external contributions at this point as we need to make progress on porting the gutenberg-mobile code to gutenberg first (see Monorepo effort). |
@cameronvoell One thing we will need to handle as well is updating our i18n script to include jetpack translations into our (mobile) bundles. I'm not familiar with how translations are extracted from source files on jetpack but if it works the same way as on gutenberg, then strings inside |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
could you maybe have a look at this one?
Before we merge this, I'd recommend the following:
- Add
native.js
to the list of things we should ignore here:
jetpack/tools/build-release-branch.sh
Lines 58 to 69 in 22eb790
echo "/extensions/**/*.css" >> .gitignore echo "/extensions/**/*.gif" >> .gitignore echo "/extensions/**/*.jpeg" >> .gitignore echo "/extensions/**/*.jpg" >> .gitignore echo "/extensions/**/*.js" >> .gitignore echo "/extensions/**/*.json" >> .gitignore echo "/extensions/**/*.jsx" >> .gitignore echo "/extensions/**/*.md" >> .gitignore echo "/extensions/**/*.png" >> .gitignore echo "/extensions/**/*.sass" >> .gitignore echo "/extensions/**/*.scss" >> .gitignore echo "/extensions/**/*.svg" >> .gitignore
Lines 66 to 79 in 90f7e1e
extensions/**/*.css extensions/**/*.gif extensions/**/*.jpeg extensions/**/*.jpg extensions/**/*.js extensions/**/*.json extensions/**/*.jsx extensions/**/*.md extensions/**/*.png extensions/**/*.sass extensions/**/*.scss extensions/**/*.svg extensions/**/*.snap extensions/.eslintrc.js - Add a mention of the files to the main Extensions' readme here:
Line 24 in 9d1d1ed
└── view.scss ← styles loaded on the frontend
Even if this is still a work in progress, it will be useful to mention it there as a12s looking to contribute new blocks may be curious about those files and what to do about them when they look at existing blocks.
Once you're all done, feel free to update the labels on the PR and we'll take a look.
Thank you!
Wouldn't the About the extensions README, it seems to me that the "Extension Structure" is more or less instructions on how a block or plugin should be organized. It doesn't list extra files such as
WDYT? |
Yeah you're right, it may do the trick!
That little snippet is perfect 👍 |
I updated the status to "Needs review" since I think we addressed the items from here: #15389 (review)
I didn't see anything clickable on CodeClimate even after logging in with my Github, so I may be missing some permission to update the status of the issues there (see screenshot): Also it seems Tug update to the README might have triggered the |
Caution: This PR has changes that must be merged to WordPress.com |
r205932-wpcom |
Changes proposed in this Pull Request:
This PR adds
.native.js
equivalents of 5 files related to the jetpack/extensionscontact-info
block in order to make thecontact-info
block usable in the gutenberg-mobile editor used by the WordPress Android and iOS apps.See related gutenberg-mobile PR: wordpress-mobile/gutenberg-mobile#1934 for integrating the jetpack block into the mobile editor from the jetpack repo.
.native.js
versions of existing files in order to enable utilizing thecontact-info
block within the mobile gutenberg editor on Android and iOS (using React Native).Is this a new feature or does it add/remove features to an existing part of Jetpack?
Testing instructions:
Proposed changelog entry for your changes: