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

Enhance/press7 60 jetpack boost #25

Open
wants to merge 25 commits into
base: main
Choose a base branch
from
Open

Enhance/press7 60 jetpack boost #25

wants to merge 25 commits into from

Conversation

AleTorrisi
Copy link

Proposed changes

This branch introduces the new section "Advanced settings" which includes main Jetpack boost options, so that they can be set by Bluehost panel and synchronized with Jetpack panel too.

Type of Change

  • Bugfix (non-breaking change which fixes an issue)
  • [x ] New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to not work as expected)
  • Documentation Update (if none of the other choices apply)

Video

Screen-2024-11-11-134622.mp4

Checklist

  • [x ] I have read the CONTRIBUTING doc
  • I have viewed my change in a web-browser
  • [x ] Linting and tests pass locally with my changes
  • I have added tests that prove my fix is effective or that my feature works
  • I have added necessary documentation (if appropriate)

Tweak: replace "console.log" with "console.error"
1764da6 
Fix: avoid import of unused component
@chrisdavidmiles
Copy link

@arnavchaudhary We need a PR on this

arunshenoy99
arunshenoy99 requested changes Nov 16, 2024
View reviewed changes
Copy link
Member

@arunshenoy99 arunshenoy99 left a comment

Choose a reason for hiding this comment

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

Hey @AleTorrisi, thank you for the PR! This looks fantastic, especially since it's your first time working on this codebase. I added a few small comments and optimizations. Please take a look and let me know if they make sense.

Other than that:

  1. Let's ensure we run the wp-scripts linter on the JS files.

  2. Let's ensure we have a newline at the end of each file for consistency.

  3. Here's what I am seeing on my local environment with your changes. If I’m missing something, that’s all OK, but please reconfirm if these buttons look correct:

    image

Let me know your thoughts or if anything needs further clarification ✌️

components/advancedSettings/JetpackBoost/InstallActivatePluginButton.js Outdated Show resolved Hide resolved
components/performance/index.js Outdated Show resolved Hide resolved
includes/Performance.php Outdated Show resolved Hide resolved
includes/Performance.php Outdated Show resolved Hide resolved
includes/RestApi/JetpackController.php Outdated Show resolved Hide resolved
components/advancedSettings/JetpackBoost/index.js Outdated Show resolved Hide resolved
components/advancedSettings/JetpackBoost/index.js Outdated Show resolved Hide resolved
components/performance/index.js Outdated Show resolved Hide resolved
includes/Performance.php Outdated Show resolved Hide resolved
components/advancedSettings/JetpackBoost/InstallActivatePluginButton.js Outdated Show resolved Hide resolved
@wpscholar wpscholar requested review from bradp and removed request for wpscholar November 16, 2024 17:38
@AleTorrisi
Tweak: groups imports by type
94f7456
@AleTorrisi
Tweak: component and variable name in "Advanced Settings" section
d4c4074
@AleTorrisi
Tweak: better handling of Rest API
319e8c9
@AleTorrisi
Tweak: handling of failure during saving options
23ed28b
@AleTorrisi
Tweak: remove unused constant
6bf3faf
@AleTorrisi
Tweak: parameter name
a569d2a 
Tweak: handling of exceptions and errors during option saving
@AleTorrisi
Tweak: import 'useState' and ''useEffect' from WordPress library inst…
3e39933 
…ead that directly from React
@AleTorrisi
Tweak: fetch options function
be0bf02
@AleTorrisi
Fix remove unused text
d1c6e62
@AleTorrisi
Tweak: store Jetpack Boost options in NewFold Runtime object avoiding…
b71cd5f 
… useless api call
@AleTorrisi
Tweak: handling of style script
b58793d
@AleTorrisi
Tweak: replaced "src/css" folder with "styles" folder
717557b 
Fix: use Performance token to manage Jetpack boost installation process
@AleTorrisi
JS Lint
70b1e1f 
phpcs
@AleTorrisi
Copy link
Author

Hey @AleTorrisi, thank you for the PR! This looks fantastic, especially since it's your first time working on this codebase. I added a few small comments and optimizations. Please take a look and let me know if they make sense.

Other than that:

  1. Let's ensure we run the wp-scripts linter on the JS files.
  2. Let's ensure we have a newline at the end of each file for consistency.
  3. Here's what I am seeing on my local environment with your changes. If I’m missing something, that’s all OK, but please reconfirm if these buttons look correct:
    image

Let me know your thoughts or if anything needs further clarification ✌️

  • JS Lint has been performed in this commit.
  • I can't replicate the issue about newline at the end of the file, it exists on my side
  • I can see just one button by my side, please launch "npm run build" and empty the browser cache.

arunshenoy99
arunshenoy99 previously approved these changes Nov 20, 2024
View reviewed changes
Copy link
Member

@arunshenoy99 arunshenoy99 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 the performance test is failing for something related to accessibility. Other than that looks good to me. Thanks! @AleTorrisi

bradp
bradp reviewed Nov 20, 2024
View reviewed changes
components/performance/defaultText.js
'wp-module-performance'
),
jetpackBoostCriticalCssTitle: __(
'Optimize Critical CSS Loading (manual)',
Copy link
Contributor

Choose a reason for hiding this comment

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

What's "manual" mean in this case? cc @chrisdavidmiles

Copy link
Contributor

@bradp bradp Nov 20, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

Might be the difference between paid Jetpack Boost vs free, on paid this is automatic.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@wpscholar wpscholar wpscholar left review comments

@bradp bradp bradp left review comments

@arunshenoy99 arunshenoy99 arunshenoy99 left review comments

At least 1 approving review is required to merge this pull request.

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@AleTorrisi @chrisdavidmiles @bradp @wpscholar @arunshenoy99