Skip to content

Components: Add PricingTable component #25377

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
Merged
danielpost merged 23 commits into from
Aug 17, 2022
Merged

Components: Add PricingTable component #25377

danielpost merged 23 commits into from
Aug 17, 2022

Conversation

@danielpost
Copy link
Contributor

@danielpost danielpost commented Aug 3, 2022

This PR adds a new Pricing Table component, that can be used to compare multiple plans.

Changes proposed in this Pull Request:

  • Add new Pricing Table component.

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

p1659435614747009-slack-C02TQF5VAJD
p6TEKc-6td-p2

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

No.

Testing instructions:

  • Go to Storybook, and make sure the new Pricing Table component looks like the screenshot below.
  • Run npm run test in the component package and make sure the tests pass.

CleanShot 2022-08-03 at 16 39 52@2x

@matticbot
Copy link
Contributor

matticbot commented Aug 3, 2022

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

github-actions bot commented Aug 3, 2022 •
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: September 6, 2022.
  • Scheduled code freeze: August 30, 2022.

Boost plugin:

  • Next scheduled release: September 6, 2022.
  • Scheduled code freeze: August 29, 2022.

Social plugin:

  • Next scheduled release: September 6, 2022.
  • Scheduled code freeze: August 29, 2022.

Starter Plugin plugin:

  • Next scheduled release: September 6, 2022.
  • Scheduled code freeze: August 29, 2022.

Protect plugin:

  • Next scheduled release: September 6, 2022.
  • Scheduled code freeze: August 29, 2022.

Videopress plugin:

  • Next scheduled release: September 6, 2022.
  • Scheduled code freeze: August 29, 2022.

@danielpost danielpost marked this pull request as ready for review August 3, 2022 14:43
@danielpost danielpost requested review from a team August 3, 2022 14:43
retrofox
retrofox reviewed Aug 3, 2022
View reviewed changes
retrofox
retrofox reviewed Aug 3, 2022
View reviewed changes
danielpost
danielpost commented Aug 3, 2022
View reviewed changes
projects/js-packages/components/components/pricing-table/index.tsx
PricingTableItemProps,
} from './types';

const PricingTableContext = createContext( undefined );
Copy link
Contributor Author

@danielpost danielpost Aug 3, 2022

Choose a reason for hiding this comment

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

Some context on why I'm adding undefined here: WordPress/gutenberg#39526

@danielpost danielpost requested a review from a team as a code owner August 4, 2022 07:54
@github-actions github-actions bot added [Plugin] Jetpack Issues about the Jetpack plugin. https://wordpress.org/plugins/jetpack/ [Plugin] Protect A plugin with features to protect a site: brute force protection, security scanning, and a WAF. [Plugin] Starter Plugin [Plugin] VideoPress A standalone plugin to add high-quality VideoPress videos to your site. labels Aug 4, 2022
@danielpost danielpost requested a review from retrofox August 4, 2022 07:58
renatoagds
renatoagds suggested changes Aug 4, 2022
View reviewed changes
projects/js-packages/components/components/pricing-table/index.tsx
const items = useContext( PricingTableContext );
const Wrapper = isLg ? Fragment : 'div';
const wrapperProps = ! isLg ? { className: styles.card } : {};
let index = 0;
Copy link
Contributor

@renatoagds renatoagds Aug 4, 2022

Choose a reason for hiding this comment

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

You can remove it, the second received parameter in Children.map is the index.

Children.map( children, ( child, index ) => ... )

Copy link
Contributor Author

@danielpost danielpost Aug 9, 2022

Choose a reason for hiding this comment

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

I've left this in, because I want the index of the PricingTableItem, not just anything in PricingTableColumn. For example, if there's a PricingTableHeader in the column, the indexes of the items will be 1 and 2 (after the headers), but the indexes of the related labels will still be 0 and 1.

That being said, there's probably a better way to do this so I'm very much open to suggestions.

renatoagds
renatoagds reviewed Aug 4, 2022
View reviewed changes
projects/js-packages/components/components/pricing-table/styles.module.scss Outdated
}

.label.label {
font-weight: 700;
Copy link
Contributor

@renatoagds renatoagds Aug 4, 2022

Choose a reason for hiding this comment

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

You can remove this one, you're already using strong there, and we avoid overriding font-weight since Text is the one responsible for it.

Btw, this is something we should go for Design, in their current specs, there's no strong variant for body-small

Copy link
Contributor Author

@danielpost danielpost Aug 9, 2022

Choose a reason for hiding this comment

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

Yeah, I think I added the strong later (that's also why I initially used .value.value and .label.label, since without those double classes the Text component was overriding it.

projects/js-packages/components/components/pricing-table/styles.module.scss Outdated
position: absolute;
height: 100%;
width: calc(
( 100% / var( --columns ) ) - ( ( var( --columns ) - 1 ) * var( --gap ) / var( --columns ) )
Copy link
Contributor

@renatoagds renatoagds Aug 4, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

IMO this approach has 2 problems:

Is difficult to read all the calc and looks really complex and could break in some moment if something changes in the structure.

I took the freedom to create a new branch and suggest a refactor at this, using display: contents:

2c0c5a2

Take a look at it, and let me know if makes sense.

It already includes: #25377 (comment) suggestion on it

Copy link
Contributor Author

@danielpost danielpost Aug 9, 2022

Choose a reason for hiding this comment

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

Should be better now!

@danielpost
Copy link
Contributor Author

danielpost commented Aug 9, 2022

@renatoagds Thank you for the thorough review! It was super helpful and I think it helped simplify and improve the component a lot.

display: contents is exactly what I needed to remove the duplication. I ended up using your branch as a base and simplified it even more. I also used clip-path to render the shadows properly on the card without having each item cast a shadow on the previous item.

Looking forward to hearing what you think!

@danielpost danielpost requested a review from renatoagds August 9, 2022 15:38
renatoagds
renatoagds previously approved these changes Aug 15, 2022
View reviewed changes
Copy link
Contributor

@renatoagds renatoagds left a comment

Choose a reason for hiding this comment

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

LGTM! Thanks for your work on this 🚀

@danielpost danielpost merged commit b9b53cb into trunk Aug 17, 2022
@danielpost danielpost deleted the add/pricing-table-component branch August 17, 2022 15:20
@github-actions
Copy link
Contributor

github-actions bot commented Aug 17, 2022

Great news! One last step: head over to your WordPress.com diff, D85299-code, and deploy it.
Once you've done so, come back to this PR and add a comment with your SVN changeset ID (e.g. r12345-wpcom).

Thank you!

@danielpost
Copy link
Contributor Author

danielpost commented Aug 17, 2022

r250813-wpcom

retrofox pushed a commit that referenced this pull request Aug 17, 2022
@danielpost @renatoagds
@sdixon194 @retrofox
[not verified] Components: Add PricingTable component (#25377)
91efd87 
Co-authored-by: Renato Augusto Gama dos Santos <renato_0603@hotmail.com>
Co-authored-by: sdixon194 <steve.dixon@automattic.com>
@bindlegirl bindlegirl mentioned this pull request Aug 31, 2022
Merged
2 tasks
@jsnmoon jsnmoon mentioned this pull request Sep 20, 2022
Closed
@dkmyta dkmyta mentioned this pull request Sep 22, 2022
Merged
8 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@retrofox retrofox Awaiting requested review from retrofox

1 more reviewer

@renatoagds renatoagds renatoagds approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

[JS Package] Components [JS Package] Connection [JS Package] IDC [JS Package] Licensing [JS Package] Partner Coupon [JS Package] Publicize Components [JS Package] Storybook [Package] Ad aka WordAds [Package] Backup [Package] My Jetpack [Package] Search Contains core Search functionality for Jetpack and Search plugins [Plugin] Boost A feature to speed up the site and improve performance. [Plugin] Jetpack Issues about the Jetpack plugin. https://wordpress.org/plugins/jetpack/ [Plugin] Protect A plugin with features to protect a site: brute force protection, security scanning, and a WAF. [Plugin] Social Issues about the Jetpack Social plugin [Plugin] Starter Plugin [Plugin] VideoPress A standalone plugin to add high-quality VideoPress videos to your site. RNA Touches WP.com Files

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

6 participants

@danielpost @matticbot @retrofox @renatoagds @sdixon194