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

Upgrade Nudge: Add generic lib, style properly #13074

Closed
wants to merge 10 commits into from

Conversation

ockham
Copy link
Contributor

@ockham ockham commented Jul 17, 2019

Fixes #13040

Changes proposed in this Pull Request:

Move the existing frontend upgrade nudge from the Simple Payments module into a shared library, parametrize it for use with other blocks, and style it akin to its editor (JS) counterpart .

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

  • Modifies an existing part

Testing instructions:

TBD

Proposed changelog entry for your changes:

TBD
(Test both the block and the widget!)

TODO

  • Need Warning component styling from @wordpress/block-editor.
  • The way I'm adding styling is currently a bit ugly. We should really build an UpgradeNudge specific CSS file (rather than piggy-backing on the view-side mechanism for SimplePayments), and enqueue it from _lib/inc/upgrade-nudge.php)

@ockham ockham added [Status] In Progress [Focus] Blocks Issues related to the block editor, aka Gutenberg, and its extensions developed in Jetpack labels Jul 17, 2019
@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 D30523-code before merging this PR. Thank you!

@jetpackbot
Copy link

jetpackbot commented Jul 17, 2019

Warnings
⚠️

Consider adding new PHP files to PHPCS whitelist for automatic linting:
_inc/lib/upgrade-nudge.php

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 e9ae350

@matticbot
Copy link
Contributor

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

3 similar comments
@matticbot
Copy link
Contributor

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

@matticbot
Copy link
Contributor

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

@matticbot
Copy link
Contributor

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

@ockham ockham force-pushed the update/paid-block-frontend-upgrade-nudge branch from 4183a40 to e9ae350 Compare July 18, 2019 11:18
@ockham
Copy link
Contributor Author

ockham commented Jul 19, 2019

I'm going to abandon this since it's kinda untenable:

  • The markup is kinda hard to reconstruct manually from the corresponding JSX components. Furthermore, those are still under active development (mostly styling and copy fixes). Each change needs to be carried over manually.
  • There's no easy way to inline the Gridicon! Our Gridicon package doesn't export individual Gridicon SVGs but only JSX components, and the "source" is a single SVG containing all of them. Embedding an individual SVG using #anchor notation would work, but we want to inline the star icon here (i.e. include the SVG markup in the HTML rendered by the PHP), so we can add additional class names to style it (color!)

Working on #13070 instead.

@ockham ockham closed this Jul 19, 2019

// TODO: Make button work

return <<<EOF
Copy link
Member

Choose a reason for hiding this comment

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

You might be saved from the rabbit hole by abandoning the goal of getting the same markup and shared CSS from the editor to work in the frontend, reasons I'm suggesting that are:

  • it's a complex process that isn't likely utilized elsewhere than these nudges
  • CSS and editor-warning is heavily geared towards working in a block-editor context; it would be very easy to miss this frontend nudge breaking because of changes in upgrade nudge made for the editor or worse, changes in Gutenberg itself.
  • You'd have to enqueue all wp-block-editor/wp-editor/wp-components/wp-editor-font (ref) styles which is a lot of CSS and none of it is scoped (things like .components-button or .DayPicker)

Since this upgrade nudge isn't probably seen very often (primary way of seeing the nudge would be in the editor), I'd suggest sticking to something simple and straightforward (HTML markup wise), similar to what current simple payments already has.

There are tons of good reasons to stick to one source of truth tho and those might very well outweigh risks. What do you think?

This same convo might've already happened somewhere else, I'm only catching up. ;-)

@ockham ockham deleted the update/paid-block-frontend-upgrade-nudge branch July 23, 2019 08:49
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
[Focus] Blocks Issues related to the block editor, aka Gutenberg, and its extensions developed in Jetpack Touches WP.com Files
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Simple payments: change X icon to star when block isn't available
5 participants