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

Build: Server-side Render UpgradeNudge for use in PHP #13070

Merged
merged 56 commits into from
Aug 2, 2019
Merged

Build: Server-side Render UpgradeNudge for use in PHP #13070

merged 56 commits into from
Aug 2, 2019

Conversation

ockham
Copy link
Contributor

@ockham ockham commented Jul 17, 2019 •
edited
Loading

Fixes #13040.

This PR adds infrastructure to server-side render individual React components during build so they can be used in PHP. The idea is that props can be passed to the component using a simplistic templating language on the server side. The benefit is that we'll be able to re-use markup and styling and don't have to duplicate code in PHP.

This is inspired by prior art in static.jsx -- see e.g.#12381 -- but hopes to apply the same principle in a cleaner, more granular way (component level).

It also adds functionality to fetch plans data from the relevant WP.com endpoint.

Changes proposed in this Pull Request:

Build: Server-side Render UpgradeNudge for use in PHP

Is this a new feature or does it add/remove features to an existing part of Jetpack?

Adds feature to existing part.

Testing instructions:

Create a new jurassic.ninja site, connect to WP.com, selecting a free plan.
Start a new post and insert the Simple Payments block. Fill in some information and publish the post.
Verify that in the frontend, you see a warning that looks like in the editor.

image

TODO

  • Styling currently ends up in static.css. We'd like a separate style file. Do we even need the rest in static.css? Can we drop that?
  • Don't import React from 'react', use @wordpress/element instead
  • Add error handling for plans fetching

Follow-up

Minify styling

/cc @sirreal

@ockham ockham self-assigned this Jul 17, 2019
@matticbot
Copy link
Contributor

Caution: This PR has changes that must be merged to WordPress.com
Hello ockham! These changes need to be synced to WordPress.com - If you 're an a11n, please commandeer, review, and approve D30515-code before merging this PR. Thank you!

@jetpackbot
Copy link
Collaborator

jetpackbot commented Jul 17, 2019 •
edited
Loading

Warnings
⚠️

pre-commit hook was skipped for one or more commits
⚠️ "Proposed changelog entry" is missing for this PR. Please include any meaningful changes

This is an automated check which relies on PULL_REQUEST_TEMPLATE. We encourage you to follow that template as it helps Jetpack maintainers do their job. If you think 'Testing instructions' or 'Proposed changelog entry' are not needed for your PR - please explain why you think so. Thanks for cooperation 🤖

Generated by 🚫 dangerJS against ce8cccc

@matticbot
Copy link
Contributor

ockham, Your synced wpcom patch D30515-code has been updated.

@ockham ockham mentioned this pull request Jul 17, 2019
Closed
5 tasks
@ockham ockham force-pushed the add/rendered-components branch 2 times, most recently from b1742fb to c5894de Compare July 19, 2019 09:56
@ockham ockham mentioned this pull request Jul 19, 2019
Closed
@ockham ockham force-pushed the add/rendered-components branch 2 times, most recently from 99de8db to cbfadd5 Compare July 23, 2019 06:47
@matticbot
Copy link
Contributor

ockham, Your synced wpcom patch D30515-code has been updated.

@ockham ockham force-pushed the add/rendered-components branch from bb7240e to dfffa6f Compare July 23, 2019 11:59
@matticbot
Copy link
Contributor

ockham, Your synced wpcom patch D30515-code has been updated.

2 similar comments
@matticbot
Copy link
Contributor

ockham, Your synced wpcom patch D30515-code has been updated.

@matticbot
Copy link
Contributor

ockham, Your synced wpcom patch D30515-code has been updated.

@ockham ockham mentioned this pull request Jul 24, 2019
Merged
@matticbot
Copy link
Contributor

ockham, Your synced wpcom patch D30515-code has been updated.

4 similar comments
@matticbot
Copy link
Contributor

ockham, Your synced wpcom patch D30515-code has been updated.

@matticbot
Copy link
Contributor

ockham, Your synced wpcom patch D30515-code has been updated.

@matticbot
Copy link
Contributor

ockham, Your synced wpcom patch D30515-code has been updated.

@matticbot
Copy link
Contributor

ockham, Your synced wpcom patch D30515-code has been updated.

@ockham @jeherve
Use esc_html_e() here instead of echo esc_html__
ba9316f 
Co-Authored-By: Jeremy Herve <jeremy@jeremy.hu>
@ockham
Copy link
Contributor Author

ockham commented Aug 1, 2019

I rebased to be able to test on RTL languages without getting bothered by that RTL bug we fixed a few days ago in master. I also fixed all PHPCS warnings and added the new files to our PHPCS whitelist; in general I think we should aim to add all new files to that whitelist so we now all new files are clean from the start and will stay that way.

TIL about the whitelist! Thanks for doing that, I installed the PHPCS extension for VS Code.

I'm seeing errors in the console right now, but I think they may be cause by Core's @wordpress/components maybe?

react-dom.js?ver=16.8.4:500 Warning: React does not recognize the `focusHistory` prop on a DOM element. If you intentionally want it to appear in the DOM as a custom attribute, spell it as lowercase `focushistory` instead. If you accidentally passed it from a parent component, remove it from the DOM element.

Seems unrelated 🤔 since I'm not using a focusHistory prop anywhere on this branch. It seems to be coming from a withFocusReturn higher-order component; I'll dig a bit more to find if maybe it's due to the way I'm stubbing things for static rendering. (I'll tackle it as a follow-up though, i.e. not blocking this PR.)

Most likely not a question for this PR, but I wonder if we should display the block content below the upgrade nudge on the site's frontend when previewing a post where you have added a block, so folks can see what the block will look like on their site once they purchase a plan.

This makes a ton of sense to me 👍 /cc @simison @mtias

I also left a few comments here and there, but nothing blocking for this PR. Everything seems to work quite nicely.

Yay, thanks!

Can I ask for another 👍 now that I've applied suggestions and added a few i18n-to-php fixes and unit tests? 😅

@ockham ockham mentioned this pull request Aug 1, 2019
Merged
jeherve
jeherve approved these changes Aug 1, 2019
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.

This looks good to me! 🚢 !

simison
simison approved these changes Aug 1, 2019
View reviewed changes
Copy link
Member

@simison simison left a comment

Choose a reason for hiding this comment

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

Nice catch with __() vs other translation functions and thanks for adding tests.

:shipit:

@ockham ockham merged commit 508024e into master Aug 2, 2019
@ockham ockham deleted the add/rendered-components branch August 2, 2019 05:14
@matticbot matticbot added [Status] Needs Changelog and removed [Status] Ready to Merge Go ahead, you can push that green button! labels Aug 2, 2019
@ockham ockham mentioned this pull request Aug 26, 2019
Merged
@sirreal sirreal mentioned this pull request Aug 27, 2019
Merged
@jeherve jeherve mentioned this pull request Dec 10, 2020
Closed
@anomiex anomiex mentioned this pull request Jul 19, 2021
Merged
anomiex added a commit that referenced this pull request Nov 12, 2021
@anomiex
jetpack: Don't set Webpack's output.pathinfo in production builds
8f63249 
There's no point to it, the generated comments just bloat the
CSS and license.txt files.

Note Webpack already defaults it to true for `mode: development` builds,
where it makes a little more sense for the comments to be present.

It's not clear why it was ever being set in the first place,
neither #12463 nor #13070 seem to have any discussion about the
additions.
@anomiex anomiex mentioned this pull request Nov 12, 2021
Merged
anomiex added a commit that referenced this pull request Nov 12, 2021
@anomiex
jetpack: Don't set Webpack's output.pathinfo in production builds (#…
b8945db 
…21727)

There's no point to it, the generated comments just bloat the
CSS and license.txt files.

Note Webpack already defaults it to true for `mode: development` builds,
where it makes a little more sense for the comments to be present.

It's not clear why it was ever being set in the first place,
neither #12463 nor #13070 seem to have any discussion about the
additions.
@jeherve jeherve mentioned this pull request Mar 3, 2022
Closed
1 task
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@simison simison simison approved these changes

@jeherve jeherve jeherve approved these changes

Assignees

@ockham ockham

Labels
Build Touches WP.com Files
Projects
None yet
Milestone
7.7
Development

Successfully merging this pull request may close these issues.

Simple payments: change X icon to star when block isn't available
5 participants
@ockham @matticbot @jetpackbot @simison @jeherve