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

feat: refactor and rename paidplancard #2367

Merged
merged 4 commits into from
Nov 1, 2023

Conversation

adrian-codecov
Copy link
Contributor

Description

I'm adding some logic to cleanup the ProPlanCard, renamed to PaidPlanCard. I initially thought it was going to be more logic but ended up being just a minor refactor after all, this one should be fairly easy.

Notable Changes

  • Renamed ProPlanCard to PaidPlanCard and everywhere it used it
  • Refactored test, included test to use team plan information
  • Got rid of the props and added independent hooks called inside that component

Screenshots

Screenshot 2023-10-31 at 9 45 27 PM

Link to Sample Entry

Legal Boilerplate

Look, I get it. The entity doing business as "Sentry" was incorporated in the State of Delaware in 2015 as Functional Software, Inc. In 2022 this entity acquired Codecov and as result Sentry is going to need some rights from me in order to utilize my contributions in this PR. So here's the deal: I retain all rights, title and interest in and to my contributions, and by keeping this boilerplate intact I confirm that Sentry can use, modify, copy, and redistribute my contributions, under Sentry's choice of terms.

Copy link

netlify bot commented Nov 1, 2023

Deploy Preview for gazebo-staging ready!

Name Link
🔨 Latest commit 8578fd3
🔍 Latest deploy log https://app.netlify.com/sites/gazebo-staging/deploys/6541d7f01a0fa3000871108b
😎 Deploy Preview https://deploy-preview-2367--gazebo-staging.netlify.app/
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site configuration.

Copy link

netlify bot commented Nov 1, 2023

Deploy Preview for gazebo-staging ready!

Name Link
🔨 Latest commit b625004
🔍 Latest deploy log https://app.netlify.com/sites/gazebo-staging/deploys/6542a23582a1a7000833ebde
😎 Deploy Preview https://deploy-preview-2367--gazebo-staging.netlify.app/
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site configuration.

Copy link
Contributor

@nicholas-codecov nicholas-codecov left a comment

Choose a reason for hiding this comment

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

One small question, and seems like some missing test coverage 👀

Comment on lines +17 to +23
const scheduledPhase = accountDetails?.scheduleDetail?.scheduledPhase
const plan = planData?.plan
const marketingName = plan?.marketingName
const benefits = plan?.benefits
const value = plan?.value
const baseUnitPrice = plan?.baseUnitPrice
const seats = plan?.planUserCount
Copy link
Contributor

Choose a reason for hiding this comment

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

Are you able to provide any default values here if these values are possibly null?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

So all of these can't be nullable as per our backend/zod schema, so we should be fine here.

But otherwise, for a scenario that applies, what default value would work here? It almost feels like if we didn't have all of these that we shouldn't show the card, or have designs reflect that state (but to be clear, they're non-nullable here)

Screenshot 2023-11-01 at 9 35 54 AM

Copy link
Contributor

Choose a reason for hiding this comment

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

Ah I see, the optional chaining made me think they were possibly nullable

@nicholas-codecov
Copy link
Contributor

nicholas-codecov commented Nov 1, 2023

Ignore me about the test coverage seemed like a runner failed 😢 should be able to restart that single runner without causing a lot more reports to be uploaded 🤞

@codecov-staging
Copy link

codecov-staging bot commented Nov 1, 2023

Codecov Report

Merging #2367 (b625004) into main (18c50f3) will increase coverage by 0.00%.
The diff coverage is 100.00%.

Impacted file tree graph

@@           Coverage Diff           @@
##             main    #2367   +/-   ##
=======================================
  Coverage   97.05%   97.06%           
=======================================
  Files         732      732           
  Lines        8806     8814    +8     
  Branches     2125     2125           
=======================================
+ Hits         8547     8555    +8     
  Misses        256      256           
  Partials        3        3           
Files Coverage Δ
...CurrentOrgPlan/CurrentPlanCard/CurrentPlanCard.jsx 100.00% <100.00%> (ø)
...Plan/CurrentPlanCard/PaidPlanCard/PaidPlanCard.jsx 100.00% <100.00%> (ø)

Continue to review full report in Codecov by Sentry.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 18c50f3...b625004. Read the comment docs.

Copy link

codecov bot commented Nov 1, 2023

Codecov Report

Merging #2367 (b625004) into main (18c50f3) will not change coverage.
The diff coverage is 100.00%.

Impacted file tree graph

@@          Coverage Diff          @@
##            main   #2367   +/-   ##
=====================================
  Coverage   97.06   97.06           
=====================================
  Files        732     732           
  Lines       8806    8814    +8     
  Branches    2169    2174    +5     
=====================================
+ Hits        8547    8555    +8     
  Misses       255     255           
  Partials       4       4           
Files Coverage Δ
...CurrentOrgPlan/CurrentPlanCard/CurrentPlanCard.jsx 100.00% <100.00%> (ø)
...Plan/CurrentPlanCard/PaidPlanCard/PaidPlanCard.jsx 100.00% <100.00%> (ø)

Continue to review full report in Codecov by Sentry.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 18c50f3...b625004. Read the comment docs.

Copy link
Contributor

@nicholas-codecov nicholas-codecov left a comment

Choose a reason for hiding this comment

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

Send it to the mooooooon 🌔

@adrian-codecov adrian-codecov merged commit 2fc2003 into main Nov 1, 2023
29 checks passed
@adrian-codecov adrian-codecov deleted the 727-team-card-for-paid-version branch November 1, 2023 19:17
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 this pull request may close these issues.

None yet

2 participants