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

Add/user licensing banner #59892

Merged
merged 25 commits into from
Jan 11, 2022
Merged

Add/user licensing banner #59892

merged 25 commits into from
Jan 11, 2022

Conversation

oikeme
Copy link
Contributor

@oikeme oikeme commented Jan 10, 2022

Changes proposed in this Pull Request

Add a banner to the checkout flow and pricing page that informs a user when they have unused licenses that can be activated.

Testing instructions

Ensure that the banner adjusts and is displayed correctly for the mobile experience with both the Jetpack Connect flow and the plans page flow.

  • Download the PR
  • Start both Calypso and Jetpack Cloud yarn start and yarn start-jetpack-cloud-p in order
  • Ensure that you have a purchase for a plan that has a license that has not yet been activated or associated with a site.

Plans Page Flow

Jetpack Connect Flow

  • Attempt to connect a disconnected site. Once on the page to authorize the connection update the url from wordpress.com to http://calypso.localhost:3000/
  • Authorize the connection and ensure that you are taken to the pricing page and that the banner indicating an available license can be activated is present. Ensure that the banner adjust correctly for mobile

Related to #

@github-actions
Copy link

github-actions bot commented Jan 10, 2022

@matticbot
Copy link
Contributor

matticbot commented Jan 10, 2022

Here is how your PR affects size of JS and CSS bundles shipped to the user's browser:

Sections (~155 bytes added 📈 [gzipped])

name                   parsed_size           gzip_size
plans                       +760 B  (+0.1%)     +155 B  (+0.1%)
jetpack-connect             +760 B  (+0.1%)     +155 B  (+0.1%)
jetpack-cloud-pricing       +760 B  (+0.2%)     +155 B  (+0.1%)

Sections contain code specific for a given set of routes. Is downloaded and parsed only when a particular route is navigated to.

Legend

What is parsed and gzip size?

Parsed Size: Uncompressed size of the JS and CSS files. This much code needs to be parsed and stored in memory.
Gzip Size: Compressed size of the JS and CSS files. This much data needs to be downloaded over network.

Generated by performance advisor bot at iscalypsofastyet.com.

@oikeme oikeme requested a review from rcanepa January 10, 2022 10:33
@matticbot matticbot added the [Status] Needs Review The PR is ready for review. This also triggers e2e canary tests and wp-desktop tests automatically. label Jan 10, 2022
@oikeme oikeme self-assigned this Jan 10, 2022
@rcanepa rcanepa changed the base branch from trunk to add/user-licensing-modal January 10, 2022 13:00
@rcanepa
Copy link
Contributor

rcanepa commented Jan 10, 2022

I made the PR point to my add/user-licensing-modal branch instead of trunk. That way, it is easier to understand your changes in isolation.


Comments

The key icon doesn't look aligned to the copy and we are missing a . at the end of the sentence, as shown in the following capture.

image

I think that the alignment issue can be fixed by relying on display: flex;.

On Jetpack Connect, the banner is too close to the Jetpack logo.

image

The banner appears regardless of having available licenses

We need to include the same checks that control the dialog/modal appearance.

Update the key icon viewbox to align it with the banner text. Also remove unnecessary nested div and fix the copy on the banner.
Remove unused css and update banner padding
Update the activate a license banner css for mobile and Jetpack connect and fix conditions under which the banner displays.
@oikeme oikeme marked this pull request as ready for review January 10, 2022 20:12
@oikeme oikeme requested a review from a team January 10, 2022 20:12
.licensing-activation-banner {
padding-top: 16px;
padding-bottom: 16px;
color: var( --studio-white );
Copy link
Contributor

Choose a reason for hiding this comment

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

The text color in design is #DCDCDE, although it looks like we don't have such a variable available. @ederrengifo, should we update the color to match the design?

Copy link
Contributor

Choose a reason for hiding this comment

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

We need to be careful with any decision because the banner is displayed in two different enviroments: cloud.jetpack.com/pricing and wordpress.com/jetpack/connect/plans.

Copy link
Contributor

Choose a reason for hiding this comment

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

I will recommend using the closer variable to that value. In our Figma library that color is called grey 5 by the way. If that's a problem because of the two different environments, then I would recommend using white by default.

Copy link
Contributor

@vitozev vitozev left a comment

Choose a reason for hiding this comment

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

All the functional tests are working as expected! I just left a few minor style-related comments.

Remove the extra space at the end of the license banner text
Update css for license activation banner for mobile and to properly wrap banner text.
Base automatically changed from add/user-licensing-modal to trunk January 11, 2022 19:19
Copy link
Contributor

@rcanepa rcanepa left a comment

Choose a reason for hiding this comment

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

LGTM.

Awesome job, Oli!

Update text color
@oikeme oikeme merged commit 6a243bc into trunk Jan 11, 2022
@oikeme oikeme deleted the add/user-licensing-banner branch January 11, 2022 20:27
@github-actions github-actions bot removed the [Status] Needs Review The PR is ready for review. This also triggers e2e canary tests and wp-desktop tests automatically. label Jan 11, 2022
@a8ci18n
Copy link

a8ci18n commented Jan 11, 2022

This Pull Request is now available for translation here: https://translate.wordpress.com/deliverables/7079494

Hi @oikeme, could you please edit the description of this PR and add a screenshot for our translators? Ideally it'd include all of the following strings:

  • Activate it now
  • You have an available product license key.

Thank you in advance!

@a8ci18n
Copy link

a8ci18n commented Jan 24, 2022

Translation for this Pull Request has now been finished.

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

Successfully merging this pull request may close these issues.

None yet

6 participants