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

Use toLocaleString with usage of credits #12236

Merged
merged 2 commits into from
Aug 23, 2022
Merged

Use toLocaleString with usage of credits #12236

merged 2 commits into from
Aug 23, 2022

Conversation

filiptronicek
Copy link
Member

@filiptronicek filiptronicek commented Aug 19, 2022

Description

Make bigger amounts of used credits more easily readable by using the browsers locale to format the number. This also solved an issue where trailing zeros would appear if the number was for example exactly .2. This may be intentional though and if so, it is easy to fix.

This change does not change how values are rounded, both this method and the previous one do it in the same way.

Related Issue(s)

None I am aware of.

How to test

TBD. Easier to test in prod 😆

Release Notes

NONE

Documentation

Werft options:

  • /werft with-preview

@filiptronicek filiptronicek requested a review from a team August 19, 2022 19:47
@github-actions github-actions bot added the team: webapp Issue belongs to the WebApp team label Aug 19, 2022
@werft-gitpod-dev-com
Copy link

started the job as gitpod-build-ft-localedcredits.1 because the annotations in the pull request description changed
(with .werft/ from main)

@@ -114,7 +114,7 @@ function TeamUsage() {
const calculateTotalUsage = () => {
let totalCredits = 0;
billedUsage.forEach((session) => (totalCredits += session.credits));
return totalCredits.toFixed(2);
return totalCredits.toLocaleString(undefined, { maximumFractionDigits: 2 });
Copy link
Member

Choose a reason for hiding this comment

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

I think this was meant to be fixed, but was not part of those discussions.

Deferring review to @gtsiolis 🏀

@gtsiolis
Copy link
Contributor

gtsiolis commented Aug 22, 2022

/werft run with-preview with-payment

👍 started the job as gitpod-build-ft-localedcredits.2
(with .werft/ from main)

@gtsiolis
Copy link
Contributor

gtsiolis commented Aug 22, 2022

Spinning up a preview environment above[1] and will try to test this there.

For visual changes like this, it would be really helpful in the future include 🅰️ some BEFORE / AFTER screenshots and 🅱️ clearer steps to test and reproduce inside the preview environment. 💡

If testing this in a preview environment is impossible for some reason, listing the visual changes clearly with text could also suffice. ✔️

Cc @filiptronicek @geropl

@filiptronicek
Copy link
Member Author

filiptronicek commented Aug 22, 2022

Thanks @gtsiolis for the feedback 🙏!

I 💯 agree about your points on testing and screenshots, I didn't include them since I didn't find a proper way to test these changes myself (the billing I suppose is a bit harder to set up).

I see the with-payment flag listed, didn't know about that one 👀.

@gtsiolis
Copy link
Contributor

gtsiolis commented Aug 22, 2022

I didn't include them since I didn't find a proper way to test these changes myself

@filiptronicek It's ok! I've also found out about this by accident while reviewing PRs relevant to the billing feature. We could certainly be more intentional and better at documenting such things. We already have a support matrix (internal) in Product Engineering / Preview Environments, but I don't think this clearly lists the available commands for /werft.

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.

From what I understand by reading the PR description and after testing in the preview environment, this does not introduce any visual changes. @filiptronicek Let me know if this is not the case.

Production Preview Environment
credtis-production credtis-preview

@gtsiolis
Copy link
Contributor

/unhold

Copy link
Member

@geropl geropl left a comment

Choose a reason for hiding this comment

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

✔️ to unblock,

hold bc would love to see this suggestion in.

Co-authored-by: Gero Posmyk-Leinemann <32448529+geropl@users.noreply.github.com>
@gtsiolis
Copy link
Contributor

Thanks, @filiptronicek @geropl! Removing the hold.

/unhold

@roboquat roboquat merged commit 0d5ef36 into main Aug 23, 2022
@roboquat roboquat deleted the ft/localedcredits branch August 23, 2022 09:43
@roboquat roboquat added deployed: webapp Meta team change is running in production deployed Change is completely running in production labels Aug 29, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
deployed: webapp Meta team change is running in production deployed Change is completely running in production release-note-none size/XS team: webapp Issue belongs to the WebApp team
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants