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

Boost: Remove global state from Pagination component #33110

Merged
merged 7 commits into from
Sep 20, 2023

Conversation

dilirity
Copy link
Member

@dilirity dilirity commented Sep 15, 2023

Proposed changes:

  • Moves global state to parent component in favor of props;
  • Update next/previous buttons to use a separate component.

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

pc9hqz-286-p2

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

no

Testing instructions:

  • Make sure ISA Pagination is working as expected.
    CleanShot 2023-09-18 at 16 56 59

@dilirity dilirity added [Status] Needs Team Review [Plugin] Boost A feature to speed up the site and improve performance. [Boost Feature] Image Size Analysis labels Sep 15, 2023
@dilirity dilirity requested a review from a team September 15, 2023 13:18
@dilirity dilirity self-assigned this Sep 15, 2023
@github-actions
Copy link
Contributor

github-actions bot commented Sep 15, 2023

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.
  • ✅ 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 reviewed, it can then be merged.
If you need an extra review from someone familiar with the codebase, you can update the labels from "[Status] Needs Team Review" to "[Status] Needs Review", and in that case Jetpack Approvers will do a final review of your PR.


Boost plugin:

  • Next scheduled release: October 3, 2023.
  • Scheduled code freeze: September 25, 2023.

Copy link
Member

@pyronaur pyronaur left a comment

Choose a reason for hiding this comment

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

Clicking left and right arrows no longer works because the connection to current reactivity is now lost.

@pyronaur
Copy link
Member

To fix that, I think <button> needs to become a <Link> instead and link to the relevant page.

@github-actions github-actions bot added [Action] Repo Gardening Github Action: manage PR and issues in your Open Source project [Block] AI Assistant [Block] AI Chat [Block] Blogroll [CRM] Portal Module [CRM] Woo Sync Module [Extension] AI Content Lens [JS Package] Publicize Components [Package] VideoPress [Plugin] CRM Issues about the Jetpack CRM plugin [Plugin] Jetpack Issues about the Jetpack plugin. https://wordpress.org/plugins/jetpack/ [Plugin] Social Issues about the Jetpack Social plugin [Tests] Includes Tests Actions GitHub actions used to automate some of the work around releases and repository management Admin Page React-powered dashboard under the Jetpack menu RNA labels Sep 18, 2023
@dilirity dilirity force-pushed the remove/boost/state/pagination-component branch from e6fd13f to 5c2bc52 Compare September 18, 2023 11:50
@dilirity dilirity removed Admin Page React-powered dashboard under the Jetpack menu Actions GitHub actions used to automate some of the work around releases and repository management [Plugin] Jetpack Issues about the Jetpack plugin. https://wordpress.org/plugins/jetpack/ labels Sep 18, 2023
@dilirity
Copy link
Member Author

dilirity commented Sep 18, 2023

@pyronaur great catch - thanks!

I've updated the buttons to use the Link component.

I've also updated their behavior - they'll hide when necessary, instead of always showing. I kind of felt it didn't make sense to show them and disable them, as they are very tiny.

pyronaur
pyronaur previously approved these changes Sep 19, 2023
Copy link
Member

@pyronaur pyronaur left a comment

Choose a reason for hiding this comment

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

Looks like this might work. Although you could use slot instead of if right check, removing a condition - would make the component a bit simpler imho.

But it's fine if you prefer to keep it this way.

@dilirity
Copy link
Member Author

dilirity commented Sep 19, 2023

Thanks for the suggestion @pyronaur! I've updated the pagination arrow component to use slots in 02584d3.

@dilirity dilirity requested a review from pyronaur September 19, 2023 11:49
Copy link
Member

@pyronaur pyronaur left a comment

Choose a reason for hiding this comment

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

Looks good! :shipit:

@dilirity dilirity merged commit 7c99e8c into trunk Sep 20, 2023
50 of 51 checks passed
@dilirity dilirity deleted the remove/boost/state/pagination-component branch September 20, 2023 11:44
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
[Boost Feature] Image Size Analysis [Plugin] Boost A feature to speed up the site and improve performance.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants