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

Experiment: Allow SVG icons to be modified for premium blocks #16485

Closed
wants to merge 1 commit into from

Conversation

mreishus
Copy link
Contributor

Related to issue #44048.

This approach is to change simple JSX icons into stateless functional
components rendering an extra svgExtra prop. This allows extra SVG
drawing commands to be injected into each icon.

In this experimental commit, I've only modified 2 icons to take the new
svgExtra prop, but we would want to modify all jetpack icons.

The main code in register-jetpack-block.js, when registering each block,
checks to see if it's a paid block using requiresPaidPlan(). If it's a
paid block, then we "render" the icon by calling the callback, and
providing a premium star as the svgExtra prop.

NOTE: In my testing, requiresPaidPlan was always returning false, so I
shortcircuited the test to always return true in this experimental
commit.

This is a way to add the star to any icon we please, while only using
one SVG element; but we have to make each icon a bit more extensible
first.

In this experiment, "Podcast Player" and "Pay with Paypal" appear with red circles.

Related to issue #44048.

This approach is to change simple JSX icons into stateless functional
components rendering an extra `svgExtra` prop.  This allows extra SVG
drawing commands to be injected into each icon.

In this experimental commit, I've only modified 2 icons to take the new
`svgExtra` prop, but we would want to modify all jetpack icons.

The main code in register-jetpack-block.js, when registering each block,
checks to see if it's a paid block using requiresPaidPlan().  If it's a
paid block, then we "render" the icon by calling the callback, and
providing a premium star as the `svgExtra` prop.

NOTE: In my testing, requiresPaidPlan was always returning false, so I
shortcircuited the test to always return true in this experimental
commit.

This is a way to add the star to any icon we please, while only using
one SVG element; but we have to make each icon a bit more extensible
first.
@matticbot
Copy link
Contributor

Caution: This PR has changes that must be merged to WordPress.com
Hello mreishus! These changes need to be synced to WordPress.com - If you 're an a11n, please commandeer and confirm D46402-code works as expected before merging this PR. Once this PR is merged, please commit the changes to WP.com. Thank you!
This revision will be updated with each commit to this PR

@jetpackbot
Copy link

Warnings
⚠️

The PR is missing at least one [Status] label. Suggestions: [Status] In Progress, [Status] Needs Review

⚠️ "Testing instructions" are missing for this PR. Please add some
⚠️ "Proposed changelog entry" is missing for this PR. Please include any meaningful changes
⚠️ The Privacy section is missing for this PR. Please specify whether this PR includes any changes to data or privacy.

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 🤖

E2E results is available here (for debugging purposes): https://jetpack-e2e-dashboard.herokuapp.com/pr-16485

Generated by 🚫 dangerJS against 37ab444

@mreishus mreishus closed this Jul 15, 2020
@mreishus mreishus deleted the experiment-modify-svg-block-icons branch July 15, 2020 19:02
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants