-
Notifications
You must be signed in to change notification settings - Fork 1.3k
[usage] Show workspace and user details #12135
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
Conversation
/werft run 👍 started the job as gitpod-build-lau-ws-details-11640.4 |
8100853
to
493309b
Compare
eb8c456
to
45f3a55
Compare
const extendedResult = await Promise.all( | ||
billedUsageResult.map(async (res) => { | ||
const ws = await server.getWorkspaceForBillableSession(res.workspaceId, attributionId); | ||
if (!res.userId) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Comments:
- I decided on making the extra calls here instead of extending the
listBilledUsage
in the server becauselistBilledUsage
is currently being called somewhere else as well (when we check the spending limit). That call will be replaced eventually, though. Maybe we can move the calls then, but go with this in this iteration? - Happy to get a review on how to write this convoluted block better 🙏🏽
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@laushinka Now I see that we meant with the additional calls. I don't like that they introduce extra API, and really thing the perfect spot for it would be here.WDYT?
Biggest benefit I see is that we would resolve the 1+N situation, and can try to replace it with a single SQL if we do it right.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Agree there would be best - only did this as a temporary solution as I mentioned above because the spending limit also calls listBilledUsage
for now.
Will make the change.
45f3a55
to
a7373f1
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is awesome - thanks @laushinka
Suggestions for additional polish (non-blocking)
- In dark mode the header background should be a darker grey (like you get when you hover over it) and the first line of text in each table row should be lighter than the 2nd line (to make it match readability/emphasis of the first line of each row in light mode)
- In the Credits column, the 2nd line (x.x min) should not wrap - but rather use truncate with ellipses like you did for the 1st line of the Timestamp column.
approve and
/hold for technical review
a7373f1
to
071cfe9
Compare
659738b
to
1cb0f58
Compare
Looking at this now! 👀 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks great, @laushinka!
Regarding the question about the icon in #12135 (comment), I accidentally had the option to show the background in exports disabled while working on the designs, see screenshot.
It's fixed now, but here's also the SVG optimized ready to use:
<svg xmlns="http://www.w3.org/2000/svg" fill="none" viewBox="0 0 16 16">
<rect width="16" height="16" fill="#D6D3D1" rx="8"/>
<path fill="#A8A29E" fill-rule="evenodd" d="M8.74323 2.03089c.23787.08748.39963.34485.39963.63583v3.33334h2.28574c.213 0 .4084.13831.5068.3588.0984.22049.0835.48654-.0387.69018L7.89671 13.7157c-.14303.2384-.40206.341-.63993.2536-.23787-.0875-.39963-.3449-.39963-.6359v-3.3333H4.57143c-.21307 0-.40845-.13832-.50684-.35881-.0984-.22049-.08348-.48654.03871-.69019l4-6.66669c.14303-.23838.40205-.341.63993-.25352Z" clip-rule="evenodd"/>
</svg>
🍊 🍊 🍊 🍊
Regarding UX, I couldn't see this in actions as I'm still not an owner of the Gitpod team on this preview environment, but here are some comments from what I can see in the screenshots:
- It would be great if we could use some bolder fonts and colors as seen in the initial design specs as this can really make a difference in information hierarchy, see screenshot below
- Ideally, the custom icon for the prebuild entry would take the same space and use the same margins as the user avatars in the rest of the list.
CURRENT | DESIGN SPECS |
---|---|
![]() |
![]() |
@gtsiolis As Laurie is out today I tried to address all you points above ☝️ Does this work: usage view? (also, made you owner) ✔️ |
Taking another look now! 👀 Thanks, @geropl! |
b15ad85
to
002c226
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for making the changes, @geropl!
We're almost there! 🏁
Made some more changes on the fly while having the preview environment up and running. Mostly trivial color and spacing issues.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@geropl I left a couple more comments on the changes.
<ItemsList className="mt-2 text-gray-500"> | ||
<Item header={false} className="grid grid-cols-5 bg-gray-100 mb-5"> | ||
<ItemField className="my-auto"> | ||
<ItemsList className="mt-2 text-gray-400 dark:text-gray-500"> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
thought: We really need to take this component to the next iteration.
/werft run with-clean-slate-deployment 👍 started the job as gitpod-build-lau-ws-details-11640.33 |
deployment was broken before, did you notice @gtsiolis ? Finally deployed now: Do we still need your new suggestions? if yes, I'm happy to merge (later tonight, have to jumpt). |
@geropl Oh, I didn't notice, let me check again. Could you make me a team owner? |
Done. |
@geropl I just checked. Besides the margin comment in #12135 (comment), the other three comments in #12135 (review) are still relevant. |
@gtsiolis Merged and deployed ✨ : |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Left some more minor issues, but we can bundle them with #12221.
Co-authored-by: George Tsiolis <tsiolis.g@gmail.com>
41c7864
to
2afbfa3
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, tested and works! ✔️ 🙃
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
UX LGTM. Let's ship this! ⛵
/unhold |
Description
Shows the workspace context URL and user details.
Prebuilds don't have user details, so they are shown by a Gitpod name and generic icon.
Related Issue(s)
Fixes #11640
How to test
Release Notes
Documentation
Werft options: