Skip to content

[dashboard] it makes more sense to have the green reflect the "remaining hours" instead of "cosumed". Having the "consumed" it creates confusion. see issue 9702 #9716

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

Closed

Conversation

ionutale
Copy link

@ionutale ionutale commented May 3, 2022

What type of PR is this?

/kind bug

What this PR does / why we need it:

UX improvements for plans remained hours

Which issue(s) this PR fixes:

plan remaining hours and progress bar #9702

Special notes for your reviewer:

…ing hours" instead of "cosumed". Having the "consumed" it creates confusion. see issue 9702
@ionutale ionutale requested a review from a team May 3, 2022 10:14
@roboquat
Copy link
Contributor

roboquat commented May 3, 2022

@ionutale: Adding the "do-not-merge/release-note-label-needed" label because no release-note block was detected, please follow our release note process to remove it.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

@ionutale
Copy link
Author

ionutale commented May 3, 2022

Release Notes:

  • Aligned the remaining hours text with the green color of the progress bar

@ionutale
Copy link
Author

ionutale commented May 3, 2022

/uncc

@ionutale
Copy link
Author

ionutale commented May 3, 2022

/approve

@ionutale
Copy link
Author

ionutale commented May 3, 2022

/retest

@gtsiolis
Copy link
Contributor

gtsiolis commented May 3, 2022

/werft run

👍 started the job as gitpod-build-plan-remaining-hours-and-9702-fork.0
(with .werft/ from main)

@ionutale
Copy link
Author

ionutale commented May 3, 2022

/werft run

thankyou @gtsiolis

Copy link
Contributor

@gtsiolis gtsiolis left a comment

Choose a reason for hiding this comment

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

Thanks for contributing, @ionutale! 🍊

You'll also need to sign a Contributor License Agreement (CLA)[1] once before merging your first contribution. Looping in @meysholdt to reach out about the CLA. 🏓

Left one comment above, let me know what you think!

value={currentPlan.hoursPerMonth - accountStatement.remainingHours}
max={currentPlan.hoursPerMonth}
/>
<progress value={accountStatement.remainingHours} max={currentPlan.hoursPerMonth} />
Copy link
Contributor

@gtsiolis gtsiolis May 3, 2022

Choose a reason for hiding this comment

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

suggestion: I was thinking we could a) make this more useful for the upcoming usage-based pricing, b) keep the existing approach, c) add some clarity to the copy, d) avoid the negative aspects that could be attached with the draining hours progress, e) improve accessibility on this area with removing the tooltip used here by making this look like the following:

BEFORE AFTER
usage-before usage-after

What do you think? Cc @jankeromnes since we've worked together in the initial implementation in #3550.

@gtsiolis
Copy link
Contributor

gtsiolis commented May 3, 2022

/werft with-payment

☝️ Trying this for the first time!

👎 unknown command: with-payment
Use /werft help to list the available commands

@gtsiolis
Copy link
Contributor

gtsiolis commented May 3, 2022

/werft run with-payment

👍 started the job as gitpod-build-plan-remaining-hours-and-9702-fork.1
(with .werft/ from main)

☝️ Trying this for the first time! Cc @jankeromnes

@gtsiolis
Copy link
Contributor

gtsiolis commented May 4, 2022

/werft run with-payment

👍 started the job as gitpod-build-plan-remaining-hours-and-9702-fork.2
(with .werft/ from main)

@gtsiolis
Copy link
Contributor

gtsiolis commented May 4, 2022

/werft run without-vm with-helm with-payment

👍 started the job as gitpod-build-plan-remaining-hours-and-9702-fork.3
(with .werft/ from main)

@ionutale
Copy link
Author

@gtsiolis I see this PR that has not been merged yet. Do I need to do something else?

@meysholdt
Copy link
Member

hi @ionutale! I've emailed our CLA your way. I believe that is still missing before we can merge this PR. Apologies for sending it so late. If you have questions, feel free to reach me via [email protected].

@ionutale
Copy link
Author

hi @ionutale! I've emailed our CLA your way. I believe that is still missing before we can merge this PR. Apologies for sending it so late. If you have questions, feel free to reach me via [email protected].

hi @meysholdt , I've signed the CLA

@meysholdt meysholdt added cla: accepted ✔️ and removed do-not-merge/cla-pending CLA has not been signed labels Jun 2, 2022
@meysholdt
Copy link
Member

Thank you for signing!

@jankeromnes
Copy link
Contributor

jankeromnes commented Jun 8, 2022

Hey, thanks for improving Gitpod's dashboard, and for signing the CLA!

Let's try to get this merged.

  • Firstly, the preview environment is gone, so I'll trigger a new build
  • Secondly, let's see if we can find a good compromise on the "remaining hours" (simple 👍 but counting down could be a bit stressful) vs "workspace hours usage since (date) (numbers)" (much too long and complicated for my taste 😅) -- I'll reply on the review comments directly

/werft run

👍 started the job as gitpod-build-plan-remaining-hours-and-9702-fork.4
(with .werft/ from main)

@jankeromnes
Copy link
Contributor

jankeromnes commented Jun 8, 2022

/werft run with-clean-slate-deployment

👍 started the job as gitpod-build-plan-remaining-hours-and-9702-fork.5
(with .werft/ from main)

@jankeromnes
Copy link
Contributor

@ionutale Apologies, it looks like we broke the Gitpod build, and you don't have the fix yet in your Pull Request / branch.

In order to fix the build, could you please rebase this Pull Request?

If it helps, you could achieve this like so:

  1. Open this PR in Gitpod: https://gitpod.io/#https://github.com/gitpod-io/gitpod/pull/9716
  2. Run git pull --rebase upstream main to rebase
  3. Run git push -f origin plan-remaining-hours-and-9702 to update this PR

@ionutale
Copy link
Author

ionutale commented Jun 8, 2022

hi @jankeromnes,
followed your instructions.
please have a look at it

@gtsiolis
Copy link
Contributor

gtsiolis commented Jun 8, 2022

/werft run with-clean-slate-deployment

👍 started the job as gitpod-build-plan-remaining-hours-and-9702-fork.6
(with .werft/ from main)

@ionutale
Copy link
Author

ionutale commented Jun 8, 2022

It does not seem to be my commit that breaks the build

it looks like that is installer.raw that is breaking

will be built
build started (version 87bc31c9df6e0b202c36a1d4e5d2c6f804f3dda0)
WARNING: Kubernetes configuration file is group-readable. This is insecure. Location: /home/gitpod/.kube/config
WARNING: Kubernetes configuration file is world-readable. This is insecure. Location: /home/gitpod/.kube/config
Getting updates for unmanaged Helm repositories...
...Successfully got an update from the "https://helm.twun.io" chart repository
Saving 1 charts
Downloading docker-registry from repo https://helm.twun.io
Deleting outdated charts
/tmp/build/install-installer--raw-app.87bc31c9df6e0b202c36a1d4e5d2c6f804f3dda0
WARNING: Kubernetes configuration file is group-readable. This is insecure. Location: /home/gitpod/.kube/config
WARNING: Kubernetes configuration file is world-readable. This is insecure. Location: /home/gitpod/.kube/config
Getting updates for unmanaged Helm repositories...
...Successfully got an update from the "https://charts.bitnami.com/bitnami" chart repository
Error: can't get a valid version for repositories minio. Try changing the version constraint in Chart.yaml
sh: 3: cd: can't cd to third_party/charts/mysql/
sh: 4: cd: can't cd to third_party/charts/rabbitmq/
package build failed
Reason: exit status 2

this is the same error in the build 5 (before merging with main) and build 6 (after)

not sure if I can help with this one 😞

@gtsiolis
Copy link
Contributor

gtsiolis commented Jun 8, 2022

@ionutale Did you force pushed your changes?
I see there are no new changes after the message from @jankeromnes in #9716 (comment).

@ionutale
Copy link
Author

ionutale commented Jun 8, 2022

the force was not working, so I did it manually from the github frontend

@gtsiolis
Copy link
Contributor

gtsiolis commented Jun 8, 2022

/werft run with-clean-slate-deployment

👍 started the job as gitpod-build-plan-remaining-hours-and-9702-fork.7
(with .werft/ from main)

@ionutale
Copy link
Author

ionutale commented Jun 8, 2022

/werft run with-clean-slate-deployment

👎 not authorized

just in case i can run it 😄

@laushinka
Copy link
Contributor

laushinka commented Jun 10, 2022

/werft run

👍 started the job as gitpod-build-plan-remaining-hours-and-9702-fork.8
(with .werft/ from main)

@laushinka
Copy link
Contributor

@gtsiolis Reading @jankeromnes's comment, do we still need alignment on the wording?

@gtsiolis
Copy link
Contributor

gtsiolis commented Jun 10, 2022

@gtsiolis Reading @jankeromnes's #9716 (comment), do we still need alignment on the wording?

@laushinka Overall, I agree with @jankeromnes in #9716 (comment). The proposal in #9716 (comment) was 🅰️ also in favor of keeping the existing approach to fill the bar instead of drain it, as well as 🅱️ rephrase the text label.

I was thinking we could a) make this more useful for the upcoming usage-based pricing, b) keep the existing approach, c) add some clarity to the copy, d) avoid the negative aspects that could be attached with the draining hours progress, e) improve accessibility on this area with removing the tooltip used here by making this look like the following:

Given that, ⁣:a: we're going to improve and significantly change this page with the upcoming features from the usage based billing updates and :b: to help push this PR forward, I'd suggest to:

  1. Keep the existing filling bar
  2. Remove the tooltip entirely
  3. Rephrase the text element and break into two pieces like the following

Screenshot 2022-06-10 at 6 19 27 PM

@ionutale
Copy link
Author

i love it
image

@gtsiolis
Copy link
Contributor

gtsiolis commented Jun 10, 2022

Thanks, @ionutale! Could you make the changes according to the designs in #9716 (comment)?

For the element on the bottom, you could also experiment with adding a component variant for the Pill Label component, but it should be completely fine to skip this and ad-hoc style this element using the utility classes available.

Using the following classes could help:

/* 1st element */
font-semibold text-gray-500 dark:text-gray-300

/* 2nd element */
font-medium text-sm text-gray-500 bg-gray-100 dark:text-gray-400 dark:bg-gray-800 rounded-xl p-1 px-2 text-sm

/* 2nd element, dimmed part */
text-gray-400 dark:text-gray-500

@ionutale
Copy link
Author

give me a few days, and I will come back to you

@stale
Copy link

stale bot commented Jun 22, 2022

This pull request has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

@stale stale bot added the meta: stale This issue/PR is stale and will be closed soon label Jun 22, 2022
@stale stale bot closed this Jun 28, 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.

6 participants