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

Contact Info - Adding Feature Flag + Jetpack API Version Check #3001

Merged
merged 5 commits into from
Feb 9, 2021

Conversation

illusaen
Copy link
Contributor

@illusaen illusaen commented Jan 13, 2021

Fixes #2166

Added native bridge capabilities for iOS and Android in order to determine whether contact info should be shown or not.

Notes (@iamthomasbishop):

  1. When logged in and creating a post in a WPcom site, then switching to a self-hosted site via the top dropdown, the Contact Info and Story blocks still show up as there is no refresh. Is this acceptable?
  2. I have Contact Info Jetpack version requirement set to 9.1 currently to match Story block as I'm not sure which version to use.

Links:

Tests:

Self-hosted

  1. Run WP-iOS app.
  2. Create self hosted site on Jurassic Ninja.
  3. Create blog post.
  4. Open block editor and see that there is no contact info block available.
  5. Repeat steps 1-4 for WP-Android app.

WPcom site

  1. Run WP-iOS app.
  2. Open Jetpack enabled site.
  3. Create blog post.
  4. Open block editor and see that contact info block is available.
  5. Repeat steps 1-4 for WP-Android app.

Screenshots:

iOS Android

PR submission checklist:

  • I have considered adding unit tests where possible.
  • I have considered if this change warrants user-facing release notes more info and have added them to RELEASE-NOTES.txt if necessary.

@mzorz
Copy link
Contributor

mzorz commented Jan 18, 2021

When logged in and creating a post in a WPcom site, then switching to a self-hosted site via the top dropdown, the Contact Info and Story blocks still show up as there is no refresh. Is this acceptable?

This is a bug on iOS, as I verified it doesn't happen like this on WPAndroid. Can you file it (and perhaps fix it) on iOS please @illusaen ? What I understand should happen is that GutenbergProps should get updated when a site change happens, before loading Gutenberg again

Copy link
Contributor

@mzorz mzorz left a comment

Choose a reason for hiding this comment

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

The changes here look good to me @illusaen ! 🎉

One thing I noticed, the gutenberg git submodule commit hash wasn't updated in this PR. We should merge develop back into your branch so to update it and then also update and add the gutenberg submodule commit hash as well. I've done it in a follow up PR of mine which is targeting this PR of yours so, please hold back from merging this one until mine is merged on top of yours first 👍 (we can chat about it too to coordinate)

Nice job! :shipit:

@mzorz
Copy link
Contributor

mzorz commented Jan 18, 2021

BTW regarding this @illusaen :

I have Contact Info Jetpack version requirement set to 9.1 currently to match Story block as I'm not sure which version to use.

To check which version of Jetpack shipped with ContactInfo block originally, you can see the commit history in Jetpack
Automattic/jetpack@f6f313d
it seems it was tagged 8.5.beta https://github.com/Automattic/jetpack/releases/tag/8.5-beta
Also judging from dates it seems like that merge commit was made on April 16, 2020, and release 8.5 of jetpack was made on May 5, 2020.
(see SCREENSHOT here)
Screen Shot 2021-01-18 at 10 51 51

TODO: it would be safer to also check whether the Contact Info block on the web side of things is available from that point on (I think it would probably be the case but, just to double check).

@iamthomasbishop
Copy link
Contributor

When logged in and creating a post in a WPcom site, then switching to a self-hosted site via the top dropdown, the Contact Info and Story blocks still show up as there is no refresh. Is this acceptable?

@illusaen I'm not sure on the technical side, but in terms of behavior from the user's perspective, I would expect the list of blocks to reset to whatever the currently-selected site supports, per @mzorz's comment above.

I have Contact Info Jetpack version requirement set to 9.1 currently to match Story block as I'm not sure which version to use.

I'm not sure on this, but it looks like Mario might've given you an answer above 😄


Before we move forward, I'd like to do a final design review — would you mind creating test builds for iOS and Android so I can give it a spin?

@peril-wordpress-mobile
Copy link

peril-wordpress-mobile bot commented Jan 27, 2021

Wanna run full suite of Android and iOS UI tests? Click here and 'Approve' CI job!

@illusaen
Copy link
Contributor Author

BTW regarding this @illusaen :

I have Contact Info Jetpack version requirement set to 9.1 currently to match Story block as I'm not sure which version to use.

To check which version of Jetpack shipped with ContactInfo block originally, you can see the commit history in Jetpack
Automattic/jetpack@f6f313d
it seems it was tagged 8.5.beta https://github.com/Automattic/jetpack/releases/tag/8.5-beta
Also judging from dates it seems like that merge commit was made on April 16, 2020, and release 8.5 of jetpack was made on May 5, 2020.
(see SCREENSHOT here)
Screen Shot 2021-01-18 at 10 51 51

TODO: it would be safer to also check whether the Contact Info block on the web side of things is available from that point on (I think it would probably be the case but, just to double check).

Took your Android PR, which is set to 8.5, thanks much!!

@illusaen
Copy link
Contributor Author

illusaen commented Feb 2, 2021

Closing in favor of #3090 because there were some weird bundle changes that happened in this branch due to merge issues.

@illusaen illusaen closed this Feb 2, 2021
@mzorz
Copy link
Contributor

mzorz commented Feb 2, 2021

Reopening!

this happened
#3037 (comment)

look at the changes in this PR https://github.com/wordpress-mobile/gutenberg-mobile/pull/3037/files
so basically:

  1. This PR Audio Block - Audio Player UI  #3037 had the changes that looked weird to us in this PR but these weren’t merged (the PR was closed)
  2. but the jetpack / gutenberg repos got their stuff updated, so this PR Contact Info - Adding Feature Flag + Jetpack API Version Check #3001 got the latest stuff here
  3. given npm bundle creates the new stuff out of the other 2 repos as well, this PR has the changes that should have made it into develop if the PR at point 1 above was merged first :)

@mzorz mzorz reopened this Feb 2, 2021
…ions; splitting out dev flag removal for later PR.
@illusaen illusaen changed the title Contact Info Enabled in Production Contact Info - Adding Feature Flag + Jetpack API Version Check Feb 9, 2021
@illusaen
Copy link
Contributor Author

illusaen commented Feb 9, 2021

Changing this PR to only add in the feature flag + Jetpack API version checks; removing the dev flag change to a later PR so that I can first fix the additional bug that @iamthomasbishop found where selecting and then deselecting the Address block results in an unhideable keyboard.

@illusaen illusaen merged commit f328485 into develop Feb 9, 2021
@illusaen illusaen deleted the contact-info-api-jetpack branch February 9, 2021 20:53
@dcalhoun dcalhoun mentioned this pull request Feb 16, 2021
3 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Contact Info Jetpack enabled in Production
4 participants