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.

Sign up for GitHub

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

Jump to bottom

Licensing JS Package – fix icon positioning and width #21878

Merged
merged 1 commit into from
Nov 26, 2021
Merged

Licensing JS Package – fix icon positioning and width #21878

merged 1 commit into from
Nov 26, 2021

Conversation

rcanepa
Copy link
Contributor

@rcanepa rcanepa commented Nov 25, 2021

Asana card: 1201096622142517-as-1201427659384436

Changes proposed in this Pull Request:

  • Fix the error icon vertical position and min width.

Jetpack product discussion

  • Project Thread: p1HpG7-cSg-p2

Does this pull request change what data or activity we track or use?

  • No. It does not.

Testing instructions:

  • Spin up a site using this Jetpack branch.
  • Connect Jetpack to your A8C WordPress.com account.
  • Enable the A8c Proxy.
  • Go to https://cloud.jetpack.com/pricing.
  • Select a paid Jetpack product other than Jetpack Search.
  • Complete checkout.
  • On the "Thank you for purchasing [product-name]!" page, just stop there.. (Don't enter a site URL in the input box).
  • On your site, visit /wp-admin/admin.php?page=jetpack#/license/activation.
  • Enter the license key associated with your purchase (you can get it from SA).
  • Once the product is activated, refresh the page.
  • Try to enter the same license key again.
  • Observe the error message and icon making sure it looks like the capture below (if the error message is not long enough, you can type whatever you in there from the JS Console).

Demo – before

Screen Shot 2021-11-25 at 20 09 10

Demo – after

Screen Shot 2021-11-25 at 20 10 56

@rcanepa rcanepa added this to the jetpack/10.4 milestone Nov 25, 2021
@rcanepa rcanepa requested a review from a team November 25, 2021 23:21
@rcanepa rcanepa self-assigned this Nov 25, 2021
@matticbot
Copy link
Contributor

Caution: This PR has changes that must be merged to WordPress.com
Hello rcanepa! These changes need to be synced to WordPress.com - If you 're an a11n, please commandeer and confirm D70679-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

@github-actions github-actions bot added the RNA label Nov 25, 2021
@github-actions
Copy link
Contributor

github-actions bot commented Nov 25, 2021 •
edited
Loading

Thank you for your PR!

When contributing to Jetpack, we have a few suggestions that can help us test and review your patch:

  • ✅ Include a description of your PR changes.
  • ✅ All commits were linted before commit.
  • ✅ Add a "[Status]" label (In Progress, Needs Team Review, ...).
  • ✅ Add testing instructions.
  • ✅ Specify whether this PR includes any changes to data or privacy.
  • ✅ Add changelog entries to affected projects

This comment will be updated as you work on your PR and make changes. If you think that some of those checks are not needed for your PR, please explain why you think so. Thanks for cooperation 🤖

The e2e test report can be found here. Please note that it can take a few minutes after the e2e tests checks are complete for the report to be available.

Once your PR is ready for review, check one last time that all required checks (other than "Required review") appearing at the bottom of this PR are passing or skipped.
Then, add the "[Status] Needs Team review" label and ask someone from your team review the code.
Once you’ve done so, switch to the "[Status] Needs Review" label; someone from Jetpack Crew will then review this PR and merge it to be included in the next Jetpack release.

Jetpack plugin:

  • Next scheduled release: December 7, 2021.
  • Scheduled code freeze: November 30, 2021.

@github-actions github-actions bot added [Status] Needs Author Reply We would need you to make some changes or provide some more details about your PR. Thank you! [Plugin] Jetpack Issues about the Jetpack plugin. https://wordpress.org/plugins/jetpack/ labels Nov 25, 2021
@jeherve jeherve removed the [Status] Needs Author Reply We would need you to make some changes or provide some more details about your PR. Thank you! label Nov 26, 2021
@jeherve jeherve added [Status] Needs Author Reply We would need you to make some changes or provide some more details about your PR. Thank you! and removed [Status] Needs Team Review labels Nov 26, 2021
@rcanepa rcanepa added [Status] Needs Team Review and removed [Status] Needs Author Reply We would need you to make some changes or provide some more details about your PR. Thank you! labels Nov 26, 2021
@rcanepa rcanepa requested a review from jeherve November 26, 2021 12:59
monsieur-z
monsieur-z previously approved these changes Nov 26, 2021
View reviewed changes
Copy link
Contributor

@monsieur-z monsieur-z left a comment

Choose a reason for hiding this comment

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

There's an error due to a version mismatch in the checks, but otherwise looks as expected.

Note that I didn't see the exact same error message as shown in the captures.

@jeherve jeherve force-pushed the fix/user-licensing-error-message-icon branch from df06dd3 to 6331d0f Compare November 26, 2021 14:55
jeherve
jeherve approved these changes Nov 26, 2021
View reviewed changes
Copy link
Member

@jeherve jeherve left a comment

Choose a reason for hiding this comment

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

This is looking good, should be good to merge.

Unrelated, but I think the main CTA button on that page could probably look better:
Screenshot 2021-11-26 at 16 05 08

And on hover:

Screenshot 2021-11-26 at 16 05 20

It only seems to look good when in the "working" state for me (on latest Chrome).

@jeherve jeherve added [Status] Ready to Merge Go ahead, you can push that green button! and removed [Status] Needs Team Review labels Nov 26, 2021
@jeherve jeherve merged commit f30af19 into master Nov 26, 2021
@jeherve jeherve deleted the fix/user-licensing-error-message-icon branch November 26, 2021 15:10
@github-actions
Copy link
Contributor

Great news! One last step: head over to your WordPress.com diff, D70679-code, and commit it.
Once you've done so, come back to this PR and add a comment with your changeset ID.

Thank you!

@github-actions github-actions bot removed the [Status] Ready to Merge Go ahead, you can push that green button! label Nov 26, 2021
@rcanepa
Copy link
Contributor Author

rcanepa commented Nov 26, 2021

This is looking good, should be good to merge.

Unrelated, but I think the main CTA button on that page could probably look better.

That's very strange. On my end, the button looks as expected.

ActivationFlowButton.mp4

Could it be that you somehow were seeing an old version of the package?

@jeherve
Copy link
Member

jeherve commented Nov 26, 2021

Could it be that you somehow were seeing an old version of the package?

Yeah, that's got to be something like this; I cannot reproduce in a fresh JN site. Sorry for the false alert!

@rcanepa
Copy link
Contributor Author

rcanepa commented Nov 30, 2021 •
edited
Loading

Great news! One last step: head over to your WordPress.com diff, D70679-code, and commit it. Once you've done so, come back to this PR and add a comment with your changeset ID.

Thank you!

r235841-wpcom

davidlonjon added a commit that referenced this pull request Dec 2, 2021
@davidlonjon
Merge branch 'master' into add/boost-e2e-tests
ff0aada 
* master: (22 commits)
  Update storybook monorepo to v6.4.0 (#21897)
  Refresh the site's modules and settings after successful product activation (#21886)
  Plugin Install: Bump MC stat on success or fail. (#21893)
  Nav Unification: Use correct user capability for the Inbox menu item (#21882)
  Jetpack: Extend disconnection dialog component to handle multiple steps and accept more props (#21048)
  Disable CSSTidy property shorthand optimization in the editor or headstart (#21891)
  Connection UI: Update Composer instructions in README.md (#21890)
  Partner Compatibility: Adding a unique connection screen for customers who receive a coupon from a Jetpack partner (#21813)
  Search package: move search dashboard to package - the base (#21865)
  JITM: wrap CTA below text on small viewports (#21688)
  Licensing JS Package – fix icon positioning and width (#21878)
  JITM: new JS and CSS builder (#21874)
  Support site_intent for `/me/sites` endpoint (#21880)
  Don't render Critical CSS while generating CritCSS. (#21879)
  ConnectScreen: make button full width on small screens (#21683)
  Improve the copy of the license activation banner (#21876)
  Rename Webpack-built files back from `.min.js` and remove hashes (#21748)
  Search: Migrate more helper classes to package (#21751)
  Search: migrate/add search rest API (#21584)
  Add initial state to the connection package (#21864)
  ...
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jeherve jeherve jeherve approved these changes

@monsieur-z monsieur-z monsieur-z left review comments

Assignees

@rcanepa rcanepa

Labels
[JS Package] Licensing [Plugin] Jetpack Issues about the Jetpack plugin. https://wordpress.org/plugins/jetpack/ RNA Touches WP.com Files
Projects
None yet
Milestone
jetpack/10.4
Development

Successfully merging this pull request may close these issues.
4 participants
@rcanepa @matticbot @jeherve @monsieur-z