-
Notifications
You must be signed in to change notification settings - Fork 1.3k
Contact info api - featureConfig implementation #13804
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
Conversation
|
You can trigger optional UI/connected tests for these changes by visiting CircleCI here. |
|
You can test the changes on this Pull Request by downloading the APK here. |
|
Hey! Bumping this to |
| public static final String AZTEC_EDITOR_NAME = "aztec"; | ||
| public static final String WP_STORIES_CREATOR_NAME = "wp_stories_creator"; | ||
| public static final String WP_STORIES_JETPACK_VERSION = "9.1"; | ||
| public static final String WP_CONTACT_INFO_JETPACK_VERSION = "8.5"; |
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.
Hi @illusaen or @mzorz I have a question about the Jetpack version. I am looking at the release notes above and it specifies that the contact info block works on WP.com site and sites with JetPack 9.1 and above. But this value is 8.5 so I am just checking if this is expected i.e I am checking to see if these versions should be the same or not. Thanks 🙇
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.
Good observation @jd-alexander ! My findings tell me the first version of Jetpack shipping with that block was 8.5, so the release notes should be updated accordingly 👍 @illusaen do you want to update it?
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.
Also another question @mzorz should the block be tested with vanilla debug or vanilla release ?
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.
I think both should work as long as it's vanilla - I tested vanillaDebug only but it should be the same (the productFlavors work this way)
jd-alexander
left a comment
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.
1.When the build config value in the vanilla block was set to true with the Jetpack version at 9.3.1 the Contact Info Block was shown in the inserter.
2. When the build config value in the vanilla block was set to false with Jetpack version 9.3.1 the Contact Info Block was NOT shown.
3. When the build config value was true and the Jetpack version was rolled back to 8.4.2 (below 8.5) the Contact Info block was NOT shown.
LGTM 🚢 I didn't approve this because the gb-mobile PR needs updating and the WPiOS PR needs to be reviewed. Once those are reviewed/updated this PR can be approved and merged in.
|
Thanks so much for your review @jd-alexander !! 🙇 |
|
NOTE! Not to be merged before #13808 gets merged as we've added empty body implementations of the new callback interfaces introduced there (see 5a408ed) cc @jd-alexander |
|
See wordpress-mobile/gutenberg-mobile#3064 for issue coming up with contact info block. |
|
@jd-alexander I don't think it'll make it in -- got a bug to fix in contact info in general first. It's actually not enabled in production yet still so no huge hurry :) |
Bumped the milestone 👍 thanks for the heads up @jd-alexander |
|
Set the Contact Info block available to non-production builds only in a31c71a as per the reasons explained in this comment by @illusaen Consequently, also removed the relevant information from |
|
@jd-alexander this is finally ready for another look 😅 - the gutenberg-mobile PR is merged so I updated the reference here. Also worth noting, this is not going into production for now, relevant changes were made as explained in this #13804 (comment). 🙏 |
Understood. Thanks for the ping @mzorz I will take a look soon 👌🏾 |
jd-alexander
left a comment
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.
The code here looks good. I am going to give this another run tomorrow and then we are good to merge.
|
@illusaen moving this to draft as per conversation with @JavonDavis , let me know when there's a need to resurface this and we can catch up 👍 |
|
This should be ready @illusaen:
Also worth noting: we need to change the valuein gutenberg-mobile here from Another thing to note: given the check on WPiOS counterpart here I believe given the flag is behind this check Let me know 👍 |
Awesome thanks!!
|
This PR builds on top of #13758 and implements the FeatureConfig class.
Related Gutenberg-mobile: PR wordpress-mobile/gutenberg-mobile#3015 and wordpress-mobile/gutenberg-mobile#3001
Related Gutenberg PR: WordPress/gutenberg#28293
To test:
CASE A: flag ON for production
CASE B: flag OFF for production
WordPress/build.gradleedit this line in the vanilla flavorbuildConfigField "boolean", "CONTACT_INFO_BLOCK_AVAILABLE", "true"and set it tifalseinsteadPR submission checklist:
RELEASE-NOTES.txtif necessary.