-
Notifications
You must be signed in to change notification settings - Fork 800
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
Social: Consolidate social initial state #38606
Conversation
Are you an Automattician? Please test your changes on all WordPress.com environments to help mitigate accidental explosions.
Interested in more tips and information?
|
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 have asked some questions and added some thoughts. Might be a good idea to get together and chat about it tomorrow.
Left some thoughts, copying @gmjuhasz to add his as well. |
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.
Left a thought on the way we configure the assets.
Instead of commenting out, let's get rid of functions and variables we don't use. We can come back to this PR's history to add them back.
Done. 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.
This looks good and I think is really close. It tests well per the instructions.
I am pinging @gmjuhasz for a second look and testing. I don't know how to test using the Query Monitor plugin, so I didn't do that, but I will do that after pairing with either of you next week.
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.
Tests well just had some small questions. If this is done and we migrate all other initial state, what else do we need to deprecate?
@@ -1,3 +1,12 @@ | |||
export interface FeatureFlags { | |||
useAdminUiV1: boolean; |
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.
Do we have to add the new feature flag for the modal as well?
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.
Yes, we need to migrate all our initial state to this new consolidated one.
/** | ||
* Publicize_Script_Data class. | ||
*/ | ||
class Publicize_Script_Data { |
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 am a bit confused here what the class name "Script data" means. Could we have a more descriptive doc if this describes the class the best?
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.
Actually the package we created in #38430 was initially named as initial-state
and the class was also named as Jetpack_Initial_State
but after some discussion with Garage team, we reached the conclusion to use the word script data
which is what the initial state actually is. Also, WordPress uses the similar nomenclature for such data - wp_add_inline_script
has calls the second argument as $data
and likewise the other such functions.
public static function has_feature_flag( $feature ): bool { | ||
$flag_name = str_replace( '-', '_', $feature ); | ||
|
||
// If the option is set, use it. | ||
if ( get_option( 'jetpack_social_has_' . $flag_name, false ) ) { | ||
return true; | ||
} | ||
|
||
$constant_name = 'JETPACK_SOCIAL_HAS_' . strtoupper( $flag_name ); | ||
// If the constant is set, use it. | ||
if ( defined( $constant_name ) && constant( $constant_name ) ) { | ||
return true; | ||
} | ||
|
||
return Current_Plan::supports( 'social-' . $feature ); | ||
} | ||
} |
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.
We have this in publicize-base with my previous changes, would that be deprecated?
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.
Yes, we will eventually move to this new consolidated logic.
Yes, we will need to gradually migrate to the new initial state. We can do that in small PRs to make testing easier. After we are done doing that, we can come back to Unbundling publicize from Jetpack and Social - the idea that actually gave birth to this one. |
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.
This tests well per the instuctions and looks okay to merge
Based off of #38430, this PR serves as an example of how to use the consolidated initial state.
This is a minimal version of what changes we need to make to get the job done (https://github.com/Automattic/jetpack-reach/issues/135). For now it uses the new initial state only for the
useAdminUiV1
feature flag. Moving forward, we will need to update the initial state from the backend and also update the corresponding js logic.Proposed changes:
useAdminUiV1
feature flag on Social admin pageOther information:
Jetpack product discussion
Does this pull request change what data or activity we track or use?
Testing instructions:
jetpack build plugins/wpcomsh
andjetpack rsync wpcomsh
tomu-plugins