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.

Sign up for GitHub

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

Jump to bottom

VideoPress: Update VideoPress to use the placeholder component from js-packages #29271

Merged
merged 8 commits into from
Mar 14, 2023
Merged

VideoPress: Update VideoPress to use the placeholder component from js-packages #29271

merged 8 commits into from
Mar 14, 2023

Conversation

kozer
Copy link
Contributor

@kozer kozer commented Mar 3, 2023 •
edited

Proposed changes:

Replace Placeholder component with LoadingPlaceholder from js packages

Other information:

  • Have you written new tests for your changes, if applicable?
  • Have you checked the E2E test CI results, and verified that your changes do not break them?
  • Have you tested your changes on WordPress.com, if applicable (if so, you'll see a generated comment below with a script to run)?

Jetpack product discussion

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

Testing instructions:

  • Test videopress to check that in places where placeholder is used, there are no discrepancies using the new loading placeholder package

@kozer kozer requested a review from jeherve March 3, 2023 07:05
@kozer kozer self-assigned this Mar 3, 2023
@kozer kozer added [Status] Needs Review To request a review from Crew. Label will be renamed soon. [Package] VideoPress labels Mar 3, 2023
@github-actions
Copy link
Contributor

github-actions bot commented Mar 3, 2023 •
edited

Are you an Automattician? You can now test your Pull Request on WordPress.com. On your sandbox, run

bin/jetpack-downloader test jetpack update/videopress-placeholder

to get started. More details: p9dueE-5Nn-p2

@github-actions
Copy link
Contributor

github-actions bot commented Mar 3, 2023 •
edited

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: April 4, 2023.
  • Scheduled code freeze: March 28, 2023.

Videopress plugin:

  • Next scheduled release: April 4, 2023.
  • Scheduled code freeze: March 27, 2023.

@kozer kozer mentioned this pull request Mar 3, 2023
Closed
3 tasks
@kozer kozer added the DO NOT MERGE don't merge it! label Mar 3, 2023
@danielbachhuber danielbachhuber self-requested a review March 3, 2023 14:16
Base automatically changed from add/loading-placeholder to trunk March 3, 2023 15:05
@kozer kozer force-pushed the update/videopress-placeholder branch from af6738b to dd31cc7 Compare March 3, 2023 15:17
danielbachhuber
danielbachhuber reviewed Mar 3, 2023
View reviewed changes
Copy link
Contributor

@danielbachhuber danielbachhuber left a comment

Choose a reason for hiding this comment

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

Test videopress to check that in places where placeholder is used, there are no discrepancies using the new loading placeholder package

Where do I go to test this?

Also, can you edit the pull request title to be more descriptive?

@kozer kozer changed the title Update/videopress placeholder videopress: Update videopress to use the placeholder component from js-packages Mar 6, 2023
@kozer
Copy link
Contributor Author

kozer commented Mar 6, 2023

Test videopress to check that in places where placeholder is used, there are no discrepancies using the new loading placeholder package

Where do I go to test this?

Also, can you edit the pull request title to be more descriptive?

I'm not sure tbh. @jeherve said that he would add people in the loop who have worked on this before. It was a question I had as well, in the original pr I opened.

@jeherve jeherve requested a review from a team March 6, 2023 09:49
jeherve
jeherve previously requested changes Mar 6, 2023
View reviewed changes
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.

he would add people in the loop who have worked on this before. It was a question I had as well, in the original pr I opened.

I added Agora to this PR, they should be able to help with that.

Before they can review, could you fix the failing test here, by running tools/fixup-project-versions.sh?

Thanks!

@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 Mar 6, 2023
@dhasilva dhasilva changed the title videopress: Update videopress to use the placeholder component from js-packages VideoPress: Update VideoPress to use the placeholder component from js-packages Mar 9, 2023
@dhasilva dhasilva added [Status] Needs Team Review and removed [Status] Needs Author Reply We would need you to make some changes or provide some more details about your PR. Thank you! DO NOT MERGE don't merge it! labels Mar 9, 2023
@dhasilva
Copy link
Contributor

dhasilva commented Mar 9, 2023

Added a commit bumping the version and merging trunk. There were some changes in recent PRs causing conflicts.

dhasilva
dhasilva approved these changes Mar 9, 2023
View reviewed changes
Copy link
Contributor

@dhasilva dhasilva left a comment

Choose a reason for hiding this comment

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

Working nicely here. Thanks for the PR! 👍

2023-03-09_17-44-14

Peek.2023-03-09.17-40.mp4

@github-actions github-actions bot added [Plugin] Jetpack Issues about the Jetpack plugin. https://wordpress.org/plugins/jetpack/ [Plugin] VideoPress A standalone plugin to add high-quality VideoPress videos to your site. labels Mar 9, 2023
@dhasilva dhasilva requested a review from jeherve March 9, 2023 21:36
danielbachhuber
danielbachhuber reviewed Mar 9, 2023
View reviewed changes
Copy link
Contributor

@danielbachhuber danielbachhuber left a comment

Choose a reason for hiding this comment

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

Great work on this PR, @kozer 👍

@kozer
Copy link
Contributor Author

kozer commented Mar 10, 2023

Working nicely here. Thanks for the PR! +1

2023-03-09_17-44-14

Thank you!
One question though! Is it's ok to be merged, or do I have to wait for more team members to review that? (As I see, you added the sticker "Needs team review", and this is why I'm asking!)

@jeherve
Copy link
Member

jeherve commented Mar 13, 2023

One question though! Is it's ok to be merged, or do I have to wait for more team members to review that? (As I see, you added the sticker "Needs team review", and this is why I'm asking!)

It should be ok to merge, since you've got an approval! 👍

@jeherve jeherve dismissed their stale review March 13, 2023 12:55

Feedback addressed

@jeherve jeherve added [Status] Ready to Merge Go ahead, you can push that green button! and removed [Status] Needs Team Review labels Mar 13, 2023
@dhasilva dhasilva enabled auto-merge (squash) March 14, 2023 13:56
@dhasilva dhasilva merged commit 00cecbc into trunk Mar 14, 2023
@dhasilva dhasilva deleted the update/videopress-placeholder branch March 14, 2023 14:17
@github-actions github-actions bot removed the [Status] Ready to Merge Go ahead, you can push that green button! label Mar 14, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@danielbachhuber danielbachhuber danielbachhuber left review comments

@dhasilva dhasilva dhasilva approved these changes

@jeherve jeherve Awaiting requested review from jeherve

Assignees

@kozer kozer

Labels
[JS Package] Components [Package] VideoPress [Plugin] Jetpack Issues about the Jetpack plugin. https://wordpress.org/plugins/jetpack/ [Plugin] VideoPress A standalone plugin to add high-quality VideoPress videos to your site. RNA
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
4 participants
@kozer @dhasilva @jeherve @danielbachhuber