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(billing): unify deployment modals #804

Open
wants to merge 4 commits into
base: main
Choose a base branch
from

Conversation

jzsfkzm
Copy link
Contributor

@jzsfkzm jzsfkzm commented Feb 6, 2025

refs #628

@jzsfkzm jzsfkzm requested a review from a team as a code owner February 6, 2025 11:44
@baktun14
Copy link
Contributor

baktun14 commented Feb 6, 2025

#628 (comment)

@baktun14
Copy link
Contributor

baktun14 commented Feb 6, 2025

Do we need to remove

const createDeploymentConfirm = async (services: ServiceType[]) => {
?

@jzsfkzm jzsfkzm force-pushed the deposit-modal-for-managed branch from bb199db to 0721038 Compare February 9, 2025 23:03
@codecov-commenter
Copy link

codecov-commenter commented Feb 9, 2025

Codecov Report

Attention: Patch coverage is 0% with 32 lines in your changes missing coverage. Please review.

Project coverage is 8.72%. Comparing base (75377c2) to head (d894665).

Files with missing lines Patch % Lines
.../components/deployments/DeploymentDepositModal.tsx 0.00% 16 Missing ⚠️
...omponents/sdl/DeploymentMinimumEscrowAlertText.tsx 0.00% 10 Missing ⚠️
...web/src/components/new-deployment/ManifestEdit.tsx 0.00% 3 Missing ⚠️
...pps/deploy-web/src/components/sdl/RentGpusForm.tsx 0.00% 2 Missing ⚠️
...ploy-web/src/hooks/useManagedDeploymentConfirm.tsx 0.00% 1 Missing ⚠️

❌ Your patch status has failed because the patch coverage (0.00%) is below the target coverage (80.00%). You can increase the patch coverage or adjust the target coverage.

Additional details and impacted files
@@            Coverage Diff            @@
##             main    #804      +/-   ##
=========================================
- Coverage   11.33%   8.72%   -2.61%     
=========================================
  Files         439     427      -12     
  Lines       11594   11162     -432     
  Branches     2443    2375      -68     
=========================================
- Hits         1314     974     -340     
- Misses       9720   10014     +294     
+ Partials      560     174     -386     
Flag Coverage Δ
deploy-web 8.72% <0.00%> (+<0.01%) ⬆️
provider-proxy ?

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@jzsfkzm jzsfkzm force-pushed the deposit-modal-for-managed branch from 843d59c to 45ed920 Compare February 10, 2025 21:34
@ygrishajev
Copy link
Contributor

It feels a bit weird to have "buy more credits" not along "balance". wdut?

Image

@jzsfkzm jzsfkzm force-pushed the deposit-modal-for-managed branch 2 times, most recently from 99cda8c to b16eb77 Compare February 12, 2025 01:06
@jzsfkzm jzsfkzm force-pushed the deposit-modal-for-managed branch from b16eb77 to 11e24e6 Compare February 12, 2025 12:51
@jzsfkzm jzsfkzm force-pushed the deposit-modal-for-managed branch from 11e24e6 to 070104f Compare February 12, 2025 14:34
@jzsfkzm jzsfkzm force-pushed the deposit-modal-for-managed branch from 070104f to d894665 Compare February 12, 2025 23:45
@baktun14
Copy link
Contributor

It feels a bit weird to have "buy more credits" not along "balance". wdut?

Image

I suggest we put this "Buy credits" CTA to the bottom of the deposit modal along the other actions.

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.

4 participants