Publicize Components: Move remaining components to the package. #24464

pablinos · 2022-05-22T12:43:26Z

Changes proposed in this Pull Request:

This PR builds on #24408 and moves over the remaining components to the
package which are required by Jetpack Social.

This is mainly moving files, but with a few changes:

JSDoc has been updated to satisfy the new lint rules
Styles have been moved into the components where possible
We no longer display the message box and twitter options when the
toggle is used to disable Publicize

It also adds an option to override the URL used to manage connections. It previously updated it to use the new Jetpack redirect URL, but that will send Jetpack customers to Jetpack Cloud, and we're not ready for that yet. This way we can override it for Jetpack Social and update it for all at a later date.

Does this pull request change what data or activity we track or use?

No

Testing instructions:

As this is mainly a refactor, testing involves ensuring that the Publicize editor plugin in the Jetpack plugin still functions as before.

~~It would probably be worth waiting until #24408 is merged, and this is rebased, before testing this properly.~~ That's merged now and this has been rebased.

github-actions · 2022-05-22T12:45:33Z

Thank you for your PR!

When contributing to Jetpack, we have a few suggestions that can help us test and review your patch:

✅ Include a description of your PR changes.
⚠️ All commits were linted before commit.
✅ Add a "[Status]" label (In Progress, Needs Team Review, ...).
✅ Add testing instructions.
✅ Specify whether this PR includes any changes to data or privacy.
✅ Add changelog entries to affected projects

This comment will be updated as you work on your PR and make changes. If you think that some of those checks are not needed for your PR, please explain why you think so. Thanks for cooperation 🤖

The e2e test report can be found here. Please note that it can take a few minutes after the e2e tests checks are complete for the report to be available.

Once your PR is ready for review, check one last time that all required checks (other than "Required review") appearing at the bottom of this PR are passing or skipped.
Then, add the "[Status] Needs Team review" label and ask someone from your team review the code.
Once you’ve done so, switch to the "[Status] Needs Review" label; someone from Jetpack Crew will then review this PR and merge it to be included in the next Jetpack release.

Jetpack plugin:

Next scheduled release: June 7, 2022.
Scheduled code freeze: May 31, 2022.

… package. This PR builds on #24408 and moves over the remaining components to the package which are required by Jetpack Social. This is mainly moving files, but with a few changes: - JSDoc has been updated to satisfy the new lint rules - Styles have been moved into the components where possible - We no longer display the message box and twitter options when the toggle is used to disable Publicize

…onents

matticbot · 2022-05-24T11:07:19Z

Caution: This PR has changes that must be merged to WordPress.com
Hello pablinos! These changes need to be synced to WordPress.com - If you 're an a11n, please commandeer and confirm D81359-code works as expected before merging this PR. Once this PR is merged, please commit the changes to WP.com. Thank you!
This revision will be updated with each commit to this PR

pablinos

The WPCOM check is failing as this involves new files, and moving existing ones. I've updated the patch and we can review that separately.

I've added some notes to the code as a review, because I feel some of the additional changes might require a bit more explanation.

pablinos · 2022-05-24T11:58:55Z

projects/js-packages/publicize-components/src/components/connection/index.js

@@ -29,7 +31,11 @@ class PublicizeConnection extends Component {
 	 */
 	maybeDisplayLinkedInNotice = () =>
 		this.connectionNeedsReauth() && (
-			<Notice className="jetpack-publicize-notice" isDismissible={ false } status="error">
+			<Notice
+				className={ componentStyles[ 'publicize-notice' ] }


This, and the other CSS changes means that we can remove the editor.scss file and it makes the build a bit cleaner.

Ah, I see you've already done what I mentioned above in some places. Nice 👍🏼

pablinos · 2022-05-24T12:00:24Z

projects/js-packages/publicize-components/src/components/connection/index.js

 				checked={ enabled }
 				onChange={ this.onConnectionChange }
-				disabled={ disabled }
+				disabled={ disabled || this.connectionIsFailing() || this.connectionNeedsReauth() }


I discovered that we were unnecessarily wrapping this in a Disabled component, so I incorporated the extra checks into this disabled prop

pablinos · 2022-05-24T12:01:10Z

projects/js-packages/publicize-components/src/components/form/index.js

-		return connections.every( connection => ! connection.toggleable );
-	}
-
+	const isDisabled = () =>


Slight refactor here, and it helped make the linter happy.

pablinos · 2022-05-24T12:03:23Z

projects/js-packages/publicize-components/src/components/settings-button/index.js

@@ -34,20 +40,22 @@ export default function PublicizeSettingsButton() {
 		refresh();
 	}, refreshThreshold );

+	const connectionsUrl =
+		getJetpackData()?.publicizeConnectionsUrl ?? 'https://wordpress.com/marketing/connections/';


This is a temporary measure. We're not ready to send current Jetpack sites to Jetpack Cloud for the connections screen, so for now we'll default to Calypso, and this lets us change that with the initial state data.

pablinos · 2022-05-24T12:06:01Z

projects/js-packages/publicize-components/src/store/effects.js

@@ -19,7 +20,9 @@ import { SUPPORTED_CONTAINER_BLOCKS } from '../components/twitter';
 */
 export async function refreshConnectionTestResults() {
 	try {
-		const results = await apiFetch( { path: '/wpcom/v2/publicize/connection-test-results' } );
+		const connectionRefreshPath =
+			getJetpackData()?.connectionRefreshPath ?? '/wpcom/v2/publicize/connection-test-results';


Similarly to the Jetpack Cloud connections screen, we don't want to default to the new endpoint for WPCOM until we've moved to the new package. This allows us to configure the URL used in the initial state.

danielpost

This looks good to me! All the components still worked in my testing and Publicize worked with no issues. 👍🏼

danielpost · 2022-05-24T15:02:14Z

projects/js-packages/publicize-components/src/components/connection-icon/style.scss

+
+	.jetpack-publicize-gutenberg-social-icon,
+	.jetpack-publicize-connection-label-copy {
+		display: inline-block;
+		vertical-align: middle;
+	}


Definitely not this PR, but I wonder if we should import this CSS similar to how we do it in other places (className={ styles.icon }), so we don't have to worry about namespacing the classes.

Yes I've done that for the other components, but here I copied the extra rules over rather than doing that slightly bigger change. Definitely something that would be good to do as a follow up.

danielpost · 2022-05-24T15:03:06Z

projects/js-packages/publicize-components/src/components/connection/index.js

@@ -29,7 +31,11 @@ class PublicizeConnection extends Component {
 	 */
 	maybeDisplayLinkedInNotice = () =>
 		this.connectionNeedsReauth() && (
-			<Notice className="jetpack-publicize-notice" isDismissible={ false } status="error">
+			<Notice
+				className={ componentStyles[ 'publicize-notice' ] }


Ah, I see you've already done what I mentioned above in some places. Nice 👍🏼

…ze-components

danielpost · 2022-05-24T15:02:15Z

projects/js-packages/publicize-components/src/components/connection-icon/style.scss

+
+	.jetpack-publicize-gutenberg-social-icon,
+	.jetpack-publicize-connection-label-copy {
+		display: inline-block;
+		vertical-align: middle;
+	}


~~Definitely not this PR, but I wonder if we should import this CSS similar to how we do it in other places (className={ styles.icon }), so we don't have to worry about namespacing the classes.~~

Not sure why this posted again.

…ze-components

bindlegirl

Works for me.

jeherve · 2022-05-25T11:17:33Z

r246300-wpcom

github-actions · 2022-05-25T11:17:42Z

Great news! One last step: head over to your WordPress.com diff, D81359-code, and deploy it.
Once you've done so, come back to this PR and add a comment with your changeset ID.

Thank you!

matticbot added the Touches WP.com Files label May 22, 2022

pablinos self-assigned this May 22, 2022

pablinos added the [Status] In Progress label May 22, 2022

pablinos marked this pull request as draft May 22, 2022 12:45

pablinos added [Feature] Publicize Now Jetpack Social, auto-sharing [Extension] Publicize Block editor plugin [JS Package] Publicize Components labels May 22, 2022

github-actions bot added [Plugin] Jetpack Issues about the Jetpack plugin. https://wordpress.org/plugins/jetpack/ RNA labels May 22, 2022

pablinos mentioned this pull request May 22, 2022

Social: Create the Jetpack Social version of the Publicize panel #24466

Merged

Base automatically changed from add/publicize-js-package to master May 23, 2022 09:50

pablinos force-pushed the update/move-publicize-components branch from 5441eb8 to 4dd6971 Compare May 23, 2022 20:32

pablinos added 3 commits May 23, 2022 22:49

Update the lock file

4b85579

Update the connections URL until the Jetpack Cloud page is ready.

26bc360

Merge remote-tracking branch 'origin' into update/move-publicize-comp…

d851a76

…onents

pablinos marked this pull request as ready for review May 24, 2022 11:57

pablinos requested a review from a team May 24, 2022 11:57

pablinos commented May 24, 2022

View reviewed changes

pablinos added [Status] Needs Team Review and removed [Status] In Progress labels May 24, 2022

danielpost previously approved these changes May 24, 2022

View reviewed changes

Merge branch 'master' into update/move-publicize-components

d77143a

pablinos added [Status] Needs Review To request a review from Crew. Label will be renamed soon. and removed [Status] Needs Team Review labels May 24, 2022

Update project versions

d97a99d

pablinos dismissed danielpost’s stale review via d97a99d May 24, 2022 16:13

Merge remote-tracking branch 'origin/master' into update/move-publici…

5d2dff7

…ze-components

danielpost approved these changes May 25, 2022

View reviewed changes

Merge remote-tracking branch 'origin/master' into update/move-publici…

9124555

…ze-components

bindlegirl approved these changes May 25, 2022

View reviewed changes

bindlegirl added [Status] Ready to Merge Go ahead, you can push that green button! and removed [Status] Needs Review To request a review from Crew. Label will be renamed soon. labels May 25, 2022

bindlegirl added this to the jetpack/11.0 milestone May 25, 2022

jeherve merged commit 1f14fd4 into master May 25, 2022

jeherve deleted the update/move-publicize-components branch May 25, 2022 11:15

github-actions bot removed the [Status] Ready to Merge Go ahead, you can push that green button! label May 25, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Publicize Components: Move remaining components to the package. #24464

Publicize Components: Move remaining components to the package. #24464

pablinos commented May 22, 2022 •

edited

github-actions bot commented May 22, 2022 •

edited

matticbot commented May 24, 2022

pablinos left a comment

pablinos May 24, 2022

danielpost May 24, 2022

pablinos May 24, 2022

pablinos May 24, 2022

pablinos May 24, 2022

pablinos May 24, 2022

danielpost left a comment

danielpost May 24, 2022

pablinos May 24, 2022

danielpost May 24, 2022

danielpost May 24, 2022 •

edited

bindlegirl left a comment

jeherve commented May 25, 2022

github-actions bot commented May 25, 2022

Publicize Components: Move remaining components to the package. #24464

Publicize Components: Move remaining components to the package. #24464

Conversation

pablinos commented May 22, 2022 • edited

Changes proposed in this Pull Request:

Does this pull request change what data or activity we track or use?

Testing instructions:

github-actions bot commented May 22, 2022 • edited

matticbot commented May 24, 2022

pablinos left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

danielpost left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

danielpost May 24, 2022 • edited

Choose a reason for hiding this comment

bindlegirl left a comment

Choose a reason for hiding this comment

jeherve commented May 25, 2022

github-actions bot commented May 25, 2022

pablinos commented May 22, 2022 •

edited

github-actions bot commented May 22, 2022 •

edited

danielpost May 24, 2022 •

edited