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

(8P) Premium Blocks: Replace (paid) label with upgrade indicator #16628

Closed
obenland opened this issue Jul 9, 2020 · 14 comments · Fixed by #16629
Closed

(8P) Premium Blocks: Replace (paid) label with upgrade indicator #16628

obenland opened this issue Jul 9, 2020 · 14 comments · Fixed by #16629
Assignees

Comments

@obenland
Copy link
Member

obenland commented Jul 9, 2020

Let's replace the (paid) label with a star indicator in every premium block's icon. This task does not include adding the contextual tip shown in the screenshot.
We might need to add block-specific CSS to position the star correctly in each icon.

This might require changes in both Jetpack and FSE plugin.

Screenshot

image-2020-07-07-at-9 27 22-pm

Prior art:

Example for star usage: Donation Block

See 123-gh-dotcom-manage

@obenland
Copy link
Member Author

obenland commented Jul 9, 2020

@sfougnier @jancavan Is there a chance I could defer to you for the creation of that star SVG?

@obenland obenland changed the title FSE: Add upgrade indicator to premium blocks Jetpack/FSE: Add upgrade indicator to premium blocks Jul 9, 2020
@jancavan
Copy link

jancavan commented Jul 9, 2020

@obenland I think we already have a star icon, or at least it's showing the G2 icon Figma. Is it not in the repo?

@obenland
Copy link
Member Author

obenland commented Jul 9, 2020

It is, thanks. I updated the issue description with a reference to a pre-existing implementation.

@obenland obenland changed the title Jetpack/FSE: Add upgrade indicator to premium blocks Premium Blocks: Replace (paid) label with upgrade indicator Jul 9, 2020
@obenland obenland changed the title Premium Blocks: Replace (paid) label with upgrade indicator (8P) Premium Blocks: Replace (paid) label with upgrade indicator Jul 13, 2020
@cpapazoglou
Copy link
Contributor

cpapazoglou commented Jul 13, 2020

Previous exploration paYJgx-rs-p2

@jeryj jeryj self-assigned this Jul 14, 2020
@frontdevde
Copy link
Contributor

frontdevde commented Jul 14, 2020

Digging into this a bit, I found there was a prior attempt made in a similar direction:

https://github.com/Automattic/jetpack/pull/13490/files#diff-bb769916a770ad52c514ba7ab6609e48R54

Wondering if we could go a similar route using <Dashicon icon="star-filled" /> instead of the Ribbon and then adjusting the position of the star icon via CSS as needed.

@jancavan
Copy link

jancavan commented Jul 14, 2020

Just an FYI - the Premium visual indicators should disappear after a user has upgraded (as per @sfougnier).

See pbAPfg-EE-p2/#comment-1374

cc @obenland @Automattic/ajax

@jeryj
Copy link
Contributor

jeryj commented Jul 14, 2020

Looked into this a little with @mreishus. One idea was to use ::after to add in an SVG to the icons. This adds a circle to every icon:

.block-editor-block-icon::after {
    display: block;
    content: '';
    width: 24px;
    height: 24px;
    position: absolute;
    background-image: url("data:image/svg+xml,%3Csvg viewBox='0 0 24 24' xmlns='http://www.w3.org/2000/svg'%3E%3Ccircle cx='2' cy='2' r='2'/%3E%3C/svg%3E");
}

Screen Shot 2020-07-14 at 4 54 30 PM

Since we only want it on Paid blocks, we'd have to curate that list of blocks:

// specify which blocks we want this to show up for
.editor-block-list-item-jetpack-recurring-payments,
.editor-block-list-item-jetpack-simple-payments {
    .block-editor-block-icon::after {
        display: block;
        content: '';
        width: 24px;
        height: 24px;
        position: absolute;
        transform: translateX(100%);
        background-image: url("data:image/svg+xml,%3Csvg viewBox='0 0 24 24' xmlns='http://www.w3.org/2000/svg'%3E%3Ccircle fill='red' cx='2' cy='2' r='2'/%3E%3C/svg%3E");
    }
}

However, this only works for the block selector. It doesn't appear everywhere the block icon is used (Preview, Inline Inserter). I didn't see named classes for places like the / inline inserter, so we wouldn't be able to target those icons.

Screen Shot 2020-07-14 at 4 53 36 PM

The last option we explored was adding the Circle directly into the SVG of the icon.

export const icon = (
	<SVG xmlns="http://www.w3.org/2000/svg" viewBox="0 0 24 24" width="24" height="24">
		<Rect x="0" fill="none" width="24" height="24" />
		<G>
			<Path d="M20 4H4c-1.105 0-2 .895-2 2v12c0 1.105.895 2 2 2h16c1.105 0 2-.895 2-2V6c0-1.105-.895-2-2-2zm0 2v2H4V6h16zM4 18v-6h16v6H4zm2-4h7v2H6v-2zm9 0h3v2h-3v-2z" />
		</G>
		<Circle fill="red" cx="24" cy="3" r="4"/>
	</SVG>
);

Screen Shot 2020-07-14 at 4 58 01 PM

This gets us the red circle on every icon since it's embedded within it. The issue then is, how do we manage appending the extra SVG to the end of the icon? Some quick ideas:

  • Having multiple copies of paid icons.
  • Somehow injecting our SVG dynamically to the end of the Icon component (we didn't see a way to do this, but it would be amazing)
  • In paYJgx-rs-p2 we had explored adding a separate SVG (so the icon would be made up of two SVGs instead of one), but that was not a recommended path.
  • ???

Any other ideas? If we can append the SVG inside of the existing block icons so it's one <svg> element, then we should be good to go on displaying the star icon.

@mreishus
Copy link
Contributor

mreishus commented Jul 14, 2020

Draft pull request of an experiment of injecting SVG dynamically: #16485 ( Cleaned up branch, diff: 25b10-pb )
This would require all icon files in jetpack to be changed to accept the svgExtra "prop".

@mtias
Copy link
Member

mtias commented Jul 15, 2020

@jancavan @obenland the icon in the G2 library is unlikely to be adequate here since it uses a base grid that is too large. It seems you should custom hint an svg that is specifically designed for this use. Also looking at the mockups, it seems there's a white stroke added around the star that should also be accounted for.

@mtias
Copy link
Member

mtias commented Jul 15, 2020

@jeryj @mreishus my impression is that whether to use icons or not remains tentative, so I'd advise against any kind of abstraction (svgExtra) and suggest instead to just add or import the start icon directly in each icon you want to modify since you have access to all of them.

@jeryj
Copy link
Contributor

jeryj commented Jul 15, 2020

@mtias Thanks for your input and direction! To make sure I'm understanding clearly, are you saying we should:

  • Have two copies of the paid block icons (one with the star and one without)
  • Switch out which icon is showed depending on their plan state with requiresPaidPlan()

Going with the manual route, I think we'd try to keep all the paid icons within Jetpack and switch them out in the block registration. We'll need to figure out how to catch blocks registered via different plugins, like FSE. @retrofox and @jeherve, I believe I saw a conversation between the both of you where you were discussing how best to do this?

@cpapazoglou
Copy link
Contributor

As it seems we need to test the navigation section too and make sure the icon is shown / positioned properly

@ghost
Copy link

ghost commented Jul 17, 2020

Latest Design
The latest push here has been to switch the star icon out in favor of a dot, which will be applied in the same positioning as the star within the block icons in the inserter (as well as where ever those icons appear in the editor).

01B Premium Block Flow

01C Premium Block Flow

Application
Happy to help advise on exact placement and on what blocks this will need to appear on, although this list of blocks is what I have been testing against.

Screen Shot 2020-07-17 at 5 07 08 PM

Design Links
Flow states
Block states

@cpapazoglou
Copy link
Contributor

Current Issue covers jetpack blocks, we have 2 more pending issues for FSE and WPcom:
https://github.com/Automattic/wp-calypso/issues/44524
Automattic/wp-calypso#44352

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

8 participants