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

Connection UI: Building the Framework #18303

Merged
merged 10 commits into from
Feb 5, 2021
Merged

Conversation

sergeymitr
Copy link
Contributor

@sergeymitr sergeymitr commented Jan 11, 2021

Changes proposed in this Pull Request:

  • The new package
  • JS building configuration
  • Handy yarn commands
  • The WP admin menu link

The new commands (run from packages/connection-ui/ directory):

  • yarn build
  • yarn watch

The Jetpack command yarn build-packages was also edited to also build the Connection UI package.

Jetpack product discussion

p9dueE-1vM-p2
Related to #18182

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

No.

Testing instructions:

  1. Go to packages/connection-ui and run yarn install to install the required modules.
  2. Run yarn build, confirm no errors come up.
  3. Go to WP admin area, "Tools -> Connection Manager", you should see the "Connection Manager" header. That means the build was successful.
  4. Run yarn watch.
  5. Open the file packages/connection-ui/_inc/admin.jsx and change the Connection Manager text to something else.
  6. Wait a few seconds, reload the "Tools -> Connection Manager" page, make sure the changed text appears.
  7. Stop the watching process (Ctrl+C).
  8. Revert the file packages/connection-ui/_inc/admin.jsx to its original state.
  9. Go to the Jetpack root directory and run yarn build-packages. Confirm that it shows no errors.
  10. Once finished, reload the "Tools -> Connection Manager" page again. Make sure it reflects the latest changes.

The PR uses a deprecated function registerStore() of the @wordpress/data component.
Unfortunately, we can't replace it with createReduxStore() and register() right now, because they are not included into the latest Wordpress build of @wordpress/data.

Proposed changelog entry for your changes:

n/a

@sergeymitr sergeymitr added [Status] In Progress Connect Flow Connection banners, buttons, ... labels Jan 11, 2021
@sergeymitr sergeymitr added this to the 9.4 milestone Jan 11, 2021
@sergeymitr sergeymitr self-assigned this Jan 11, 2021
Copy link

@test-case-reminder test-case-reminder bot left a comment

Choose a reason for hiding this comment

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

Here are some suggested test cases for this PR.

Connection

  • In-place connection with free plan
  • In-place connection with paid plan
  • In-place connection with product purchase
  • Classic connection. Use Safari, or set a constant JETPACK_SHOULD_NOT_USE_CONNECTION_IFRAME to true
  • Disconnect/reconnect connection
  • Secondary user connection
  • Connection on multisite

Verify that the changes are compatible with the plugins that use the connection package.

  • WooCommerce Payments
  • Jetpack Boost
  • Previous versions of Jetpack

If you think that suggestions should be improved please edit the configuration file here. You can also modify/add test-suites to be used in the configuration file.

@matticbot
Copy link
Contributor

Caution: This PR has changes that must be merged to WordPress.com
Hello sergeymitr! These changes need to be synced to WordPress.com - If you 're an a11n, please commandeer and confirm D55178-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

@github-actions github-actions bot added the [Status] Needs Package Release This PR made changes to a package. Let's update that package now. label Jan 11, 2021
@jetpackbot
Copy link

jetpackbot commented Jan 11, 2021

Scheduled Jetpack release: February 16, 2021.
Scheduled code freeze: February 8, 2021

Thank you for the great PR description!

When this PR is ready for review, please apply the [Status] Needs Review label. If you are an a11n, please have someone from your team review the code if possible. The Jetpack team will also review this PR and merge it to be included in the next Jetpack release.

Generated by 🚫 dangerJS against dc20e24

@sergeymitr sergeymitr added [Status] Needs Review To request a review from Crew. Label will be renamed soon. and removed [Status] In Progress labels Jan 11, 2021
@kraftbj
Copy link
Contributor

kraftbj commented Jan 12, 2021

As an FYI, later this week, GC will be merging our repo reorg patch that, will among other things, allow for individual projects--like this new package--to build and commit to their mirror repo a JS-built version.

In other words, Composer clients can consume the package and get the built, ready to use JS after we merge and set things up for this package.

packages/connection-ui/package.json Outdated Show resolved Hide resolved
@jeherve jeherve added [Status] Needs Author Reply We would need you to make some changes or provide some more details about your PR. Thank you! and removed [Status] Needs Review To request a review from Crew. Label will be renamed soon. labels Jan 12, 2021
Copy link
Member

@zinigor zinigor left a comment

Choose a reason for hiding this comment

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

This is a great start, thank you! We're moving in the right direction, I have left some comments in the code, please take a look.

composer.json Outdated Show resolved Hide resolved
packages/connection-ui/package.json Outdated Show resolved Hide resolved
packages/connection-ui/_inc/admin.jsx Outdated Show resolved Hide resolved
packages/connection-ui/webpack.config.js Outdated Show resolved Hide resolved
packages/connection-ui/webpack.config.js Outdated Show resolved Hide resolved
@sergeymitr sergeymitr added [Status] In Progress and removed [Status] Needs Author Reply We would need you to make some changes or provide some more details about your PR. Thank you! labels Jan 12, 2021
package.json Outdated Show resolved Hide resolved
@jeherve jeherve removed this from the 9.4 milestone Jan 21, 2021
@sergeymitr sergeymitr added [Status] Needs Review To request a review from Crew. Label will be renamed soon. and removed [Status] In Progress labels Jan 21, 2021
@sergeymitr
Copy link
Contributor Author

Rebased to resolve a conflict with the master branch.

Copy link
Member

@zinigor zinigor left a comment

Choose a reason for hiding this comment

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

One minor change, and I think this is ready to be shipped. Thanks for working on this!

projects/plugins/jetpack/class.jetpack.php Outdated Show resolved Hide resolved
jeherve
jeherve previously requested changes Feb 3, 2021
Copy link
Member

@jeherve jeherve left a comment

Choose a reason for hiding this comment

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

I think we would need a .gitattributes in here to specify what we don't want to ship with the production package.

projects/packages/connection-ui/composer.json Show resolved Hide resolved
projects/packages/connection-ui/src/class-admin.php Outdated Show resolved Hide resolved
@jeherve jeherve added [Type] Feature Request [Status] Needs Author Reply We would need you to make some changes or provide some more details about your PR. Thank you! [Package] Connection UI This package no longer exists in the monorepo. and removed [Status] Needs Review To request a review from Crew. Label will be renamed soon. labels Feb 3, 2021
zinigor
zinigor previously approved these changes Feb 5, 2021
Copy link
Member

@zinigor zinigor left a comment

Choose a reason for hiding this comment

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

Let's get this party started!

@kraftbj kraftbj self-requested a review February 5, 2021 20:09
kraftbj
kraftbj previously approved these changes Feb 5, 2021
Co-authored-by: Brad Jorsch <anomiex@users.noreply.github.com>
Copy link
Contributor

@anomiex anomiex left a comment

Choose a reason for hiding this comment

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

Adding approval based on the previous version receiving approvals from others. Haven't reviewed myself.

@sergeymitr sergeymitr merged commit c87bef3 into master Feb 5, 2021
@sergeymitr sergeymitr deleted the add/connection-ui-build branch February 5, 2021 22:11
@kraftbj kraftbj removed the [Status] Needs Author Reply We would need you to make some changes or provide some more details about your PR. Thank you! label Feb 5, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Connect Flow Connection banners, buttons, ... [Package] Connection UI This package no longer exists in the monorepo. [Status] Needs Package Release This PR made changes to a package. Let's update that package now. [Type] Feature Request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants