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

Merged

ockham merged 56 commits intomasterfrom

add/rendered-components

Aug 2, 2019

Contributor

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.

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

ockham added [Status] In Progress Build labels

ockham self-assigned this

matticbot added the Touches WP.com Files label

Contributor

matticbot commented Jul 17, 2019

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!

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

Contributor

matticbot commented Jul 17, 2019

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

ockham added [Status] Blocked / Hold and removed [Status] In Progress labels

ockham mentioned this pull request

Block Editor: Add missing dependencies to package WordPress/gutenberg#16628

Closed

5 tasks

ockham force-pushed the add/rendered-components branch 2 times, most recently from b1742fb to c5894de Compare

July 19, 2019 09:56

ockham mentioned this pull request

Upgrade Nudge: Add generic lib, style properly #13074

Closed

ockham force-pushed the add/rendered-components branch 2 times, most recently from 99de8db to cbfadd5 Compare

July 23, 2019 06:47

Contributor

matticbot commented Jul 23, 2019

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

simison reviewed

View reviewed changes

extensions/shared/upgrade-nudge/index.jsx Outdated Show resolved Hide resolved

ockham force-pushed the add/rendered-components branch from bb7240e to dfffa6f Compare

July 23, 2019 11:59

Contributor

matticbot commented Jul 23, 2019

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

2 similar comments

Contributor

matticbot commented Jul 24, 2019

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

Contributor

matticbot commented Jul 24, 2019

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

ockham mentioned this pull request

Calypso Build: Fix typo in output-library-target Automattic/wp-calypso#34860

Merged

Contributor

matticbot commented Jul 24, 2019

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

4 similar comments

Contributor

matticbot commented Jul 24, 2019

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

Contributor

matticbot commented Jul 25, 2019

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

Contributor

matticbot commented Jul 25, 2019

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

Contributor

matticbot commented Jul 25, 2019

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

ockham added [Status] In Progress and removed [Status] Blocked / Hold [Status] In Progress labels


          Use esc_html_e() here instead of echo esc_html__

ba9316f

Co-Authored-By: Jeremy Herve <jeremy@jeremy.hu>

ockham dismissed jeherve’s stale review via

ba9316f

August 1, 2019 05:46

ockham and others added 4 commits

August 1, 2019 11:49


          Declare wp-components as a dependency of jetpack-components

0c82394

Co-Authored-By: Jeremy Herve <jeremy@jeremy.hu>


          Remove extra wp-components dependency

ed1bffe

Co-Authored-By: Jeremy Herve <jeremy@jeremy.hu>


          Add explanatory comments to Jetpack_Plan and Jetpack_Plans classes

994030e


          Another explanatory comment

02db419

ockham commented

View reviewed changes

extensions/shared/i18n-to-php.js Outdated Show resolved Hide resolved

ockham added 6 commits

August 1, 2019 13:34


          Add unit test for i18n-to-php.js

6fe8af9


          Typo

9ae24d1


          More generic

3f67b6c


          Add unit test for _n() (currently failing)

8b86719


          Add unit tests for _n() and _nx() (currently failing)

155d698


          Fix translate functions implementations

ce8cccc

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 mentioned this pull request

Upgrade Nudge: Fix redirect_to for Jetpack wp-admin #13165

Merged

jeherve approved these changes

View reviewed changes

Member

jeherve left a comment

This looks good to me! 🚢 !

simison approved these changes

View reviewed changes

Member

simison left a comment

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

ockham merged commit 508024e into master

ockham deleted the add/rendered-components branch

August 2, 2019 05:14

matticbot added [Status] Needs Changelog and removed [Status] Ready to Merge labels

jeherve removed the [Status] Needs Changelog label

ockham mentioned this pull request

Premium blocks: Add plan param to redirect URL - take 2 - simple sites #13281

Merged

sirreal mentioned this pull request

Dependencies: Fix broken prod dependencies #13343

Merged

jeherve mentioned this pull request

Blocks: deprecate the server-side rendered nudges #18054

Closed

anomiex mentioned this pull request

Update jetpack extension tests to work with update to @wordpress dependencies #20326

Merged

anomiex added a commit that referenced this pull request


          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 mentioned this pull request

jetpack: Don't set Webpack's output.pathinfo in production builds #21727

Merged

anomiex added a commit that referenced this pull request


          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 mentioned this pull request

Blocks: update logic to build frontend upgrade nudges #23214

Closed

1 task

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

Labels

Build Touches WP.com Files