Skip to content

Conversation

@dkmyta
Copy link
Contributor

@dkmyta dkmyta commented Sep 22, 2022

Description #
In an attempt to improve upon the fantastic work put in to create and update the new PricingTable component that will be used in the unification of standalone Jetpack product pricing pages, I have identified a number of minor updates that may present difficulties in particular cases and one potential area of improvement.

Updates:

  • The new discount label overlaps when price / offPrice values are too significant and/or when a currency symbol has been added

Screen Shot 2022-09-21 at 15 10 36

  • CSS for the main Gridicon component appears to be overriding the new .popover-icon element styles for the PricingTable component making the icon bold like the preceding text rather than fill: =var(--jp-gray); as intended.

Screen Shot 2022-09-21 at 15 16 39

Screen Shot 2022-09-21 at 15 17 45

Screen Shot 2022-09-22 at 12 49 08

Improvements:

  • We can take advantage of the logic used in the JetpackProductOffer component in Calypso that introduces a discount label and automatically generates the content of the label based on a number of pricing-related variables. This will allow us to bring consistency between the various dashboards that may display a discount badge so there are no discrepancies in the calculations that may lead to unnecessary consumer confusion.
    Calypso Discount Label

Other information:

  • Have you written new tests for your changes, if applicable?
  • Have you checked the E2E test CI results, and verified that your changes do not break them?

Jetpack product discussion

  • 1201069996155224-as-1202920335836514
  • p1HpG7-hMo-p2

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

  • No

Testing instructions:

  • Run npm run storybook:dev from the js-packages/storybook folder
  • Change the default price, offPrice, and currency values and verify the promo label reacts accordingly
  • Ensure the TOS component remains centered to columns 2 and 3 while decreasing viewport width until the mobile view is activated
  • Play with the various options and ensure the components handles the changes as expected
  • Load this PR using the Jetpack Beta plugin on a Jurassic Ninja site and verify that the icon tooltip popover handles and displays as expected
  • Check the Figma file to ensure the design matches

@github-actions
Copy link
Contributor

github-actions bot commented Sep 22, 2022

Are you an Automattician? You can now test your Pull Request on WordPress.com. On your sandbox, run bin/jetpack-downloader test jetpack update/pricing-table-improvements to get started. More details: p9dueE-5Nn-p2

@github-actions
Copy link
Contributor

github-actions bot commented Sep 22, 2022

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.

@renatoagds renatoagds requested a review from a team September 22, 2022 22:24
@dkmyta dkmyta marked this pull request as ready for review September 22, 2022 23:42
@dkmyta dkmyta mentioned this pull request Sep 23, 2022
2 tasks
@danielpost
Copy link
Contributor

Can we leave the option to add a promo label to a column that's not an automatically calculated percentage? In our case we're adding New! as a label to a column, so removing that option wouldn't be ideal.

@retrofox retrofox added [Status] Needs Review This PR is ready for review. and removed [Status] In Progress labels Sep 29, 2022
Copy link
Contributor

@retrofox retrofox left a comment

Choose a reason for hiding this comment

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

Nice enhancements!

I have a feedback about the label

image

:this: happens in VideoPress standalone plugin now, for instance.

I tried the following:

Screen.Recording.2022-09-29.at.09.33.17.mov

what do you think?

@bindlegirl bindlegirl added [Status] Needs Author Reply We need more details from you. This label will be auto-added until the PR meets all requirements. and removed [Status] Needs Review This PR is ready for review. labels Sep 29, 2022
@retrofox retrofox added [Status] Needs Review This PR is ready for review. and removed [Status] Needs Author Reply We need more details from you. This label will be auto-added until the PR meets all requirements. labels Sep 30, 2022
@dkmyta dkmyta force-pushed the update/pricing-table-improvements branch from 2f28ff5 to f3f25f1 Compare September 30, 2022 18:20
Copy link
Contributor

@oskosk oskosk left a comment

Choose a reason for hiding this comment

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

LGTM!

@oskosk oskosk merged commit a91fa1a into trunk Sep 30, 2022
@oskosk oskosk deleted the update/pricing-table-improvements branch September 30, 2022 20:46
@anomiex anomiex removed the [Status] Needs Review This PR is ready for review. label Oct 3, 2022
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.

9 participants