-
Notifications
You must be signed in to change notification settings - Fork 1.2k
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
[dashboard] usage view allows for flexible time range #14515
Conversation
started the job as gitpod-build-se-usage-view.42 because the annotations in the pull request description changed |
started the job as gitpod-build-se-usage-view.43 because the annotations in the pull request description changed |
/werft run with-clean-slate-deployment 👍 started the job as gitpod-build-se-usage-view.44 |
b5fcdf4
to
e0a7822
Compare
e0a7822
to
7969cbd
Compare
Nice improvement! |
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.
Tested and works nicely. 💖
/hold in case you want to handle this here, now.
7969cbd
to
da8c9ee
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.
Thanks @svenefftinge for adding this and hanging here while I was collecting some all around thoughts around the hot 🔥 topic of a date picker component.
Left some first-round comments below to keep the conversations going.
} | ||
subtitle={ | ||
<h2 className="tracking-wide flex mt-3"> | ||
<h2 className="mr-2">Period: </h2> |
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.
} | ||
subtitle={ | ||
<h2 className="tracking-wide flex mt-3"> | ||
<h2 className="mr-2">Period: </h2> |
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: The idea of having the period selection on the top of the left sidebar is to update all usage entries on the main content as well as all aggregated data on the left sidebar based on the period selection. We could use the heading subtitle for hijacking this action and filtering in a similar way also other views like workpaces per team, etc.
Idea of the top-left control for period selection |
---|
} | ||
subtitle={ | ||
<h2 className="tracking-wide flex mt-3"> | ||
<h2 className="mr-2">Period: </h2> |
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.
question: While the goal long term is to replace the primitive navigation between months with more flexible options (Last 7 days, 30 days, etc) with a flexible dropdown (see #11685 (comment)) and possibly move the tabs below the header (#10976), to keep the scope small here, what do you think of moving the period elements below inside the left sidebar? Alternatively, we could keep the custom filtering on the heading subtitle, which feels nice.
Early design for period selection | Selection on the sidebar | Selection on the heading |
---|---|---|
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.
If you let me pick, I'd go with the "Selection on the heading" version. Looks slickest to me.
Would be great if you could take a general look at how we want to do the range selection out of the scope of this PR.
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.
The selection on the heading looks slick indeed, and I've been wanting to make this heading component more useful so that form follows function (internal), see #10138 and #6764. Let's try that regardless of any updates that will take place in the left sidebar. 🤝
onChange={(date) => date && setEndDate(date.getTime())} | ||
selectsEnd | ||
startDate={new Date(startDate)} | ||
endDate={new Date(endDate)} |
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.
suggestion: Could we show the last day of a month for each month selection? Currently, for example, when selecting October 2022, the range changes to 01/10/2022 - 01/11/2022
, and would be more accurate if this would change to 01/10/2022 - 31/10/2022
.
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.
subtitle={ | ||
<h2 className="tracking-wide flex mt-3"> | ||
<h2 className="mr-2">Period: </h2> | ||
{startDateError} |
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.
issue: Probably not the same error as in the screenshot, and maybe out of the scope of the changes, but we should probably impose a limit on the the max date range gap like 180 days or something, right? A good MVC could be to surface a danger alert with the error message instead of breaking the page.
} | ||
subtitle={ | ||
<h2 className="tracking-wide flex mt-3"> | ||
<h2 className="mr-2">Period: </h2> |
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: Ideally, the custom period selection should be as close as possible with the fixed period selections.
b1f7e52
to
959e374
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.
Alright, almost there! Thanks for addressing all design-related feedback, @svenefftinge!
Left one more round of comments.
Other than that, looks great! Leave anything that looks insignificant to block from merging for a follow up PR. I can make most of the style changes in a separate PR. 🏀
} | ||
subtitle={ | ||
<h2 className="tracking-wide flex mt-3"> | ||
<h2 className="mr-2">Period: </h2> |
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.
The selection on the heading looks slick indeed, and I've been wanting to make this heading component more useful so that form follows function (internal), see #10138 and #6764. Let's try that regardless of any updates that will take place in the left sidebar. 🤝
onChange={(date) => date && setEndDate(date.getTime())} | ||
selectsEnd | ||
startDate={new Date(startDate)} | ||
endDate={new Date(endDate)} |
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.
selected={new Date(endDate)} | ||
onChange={(date) => date && setEndDate(date.getTime())} | ||
selectsEnd | ||
startDate={new Date(startDate)} |
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.
title={ | ||
<div className="flex items-baseline"> | ||
<h1 className="tracking-wide">{headerTitle}</h1> | ||
<h2 className="ml-3">(updated every 15 minutes).</h2> |
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.
question: We dropped this information, but I can open a follow up PR to add this back. Would you say it's not as useful to include in the page? If you think it's ok to include, here's the SVG. Using one gray color could suffice for the first iteration on light and dark theme.
<svg xmlns="http://www.w3.org/2000/svg" fill="none" viewBox="0 0 24 24">
<path fill="#0F172A" d="M12.75 6c0-.41421-.3358-.75-.75-.75s-.75.33579-.75.75h1.5ZM12 12h-.75c0 .4142.3358.75.75.75V12Zm4.5.75c.4142 0 .75-.3358.75-.75s-.3358-.75-.75-.75v1.5Zm3.75-.75c0 4.5563-3.6937 8.25-8.25 8.25v1.5c5.3848 0 9.75-4.3652 9.75-9.75h-1.5ZM12 20.25c-4.55635 0-8.25-3.6937-8.25-8.25h-1.5c0 5.3848 4.36522 9.75 9.75 9.75v-1.5ZM3.75 12c0-4.55635 3.69365-8.25 8.25-8.25v-1.5c-5.38478 0-9.75 4.36522-9.75 9.75h1.5ZM12 3.75c4.5563 0 8.25 3.69365 8.25 8.25h1.5c0-5.38478-4.3652-9.75-9.75-9.75v1.5ZM11.25 6v6h1.5V6h-1.5Zm5.25 5.25H12v1.5h4.5v-1.5Z"/>
</svg>
@@ -160,13 +175,67 @@ function UsageView({ attributionId }: UsageViewProps) { | |||
|
|||
const headerTitle = attributionId.kind === "team" ? "Team Usage" : "Personal Usage"; | |||
|
|||
const DateDisplay = forwardRef((arg: any, ref: any) => ( | |||
<div | |||
className="px-2 p-1 text-gray-500 bg-gray-100 dark:text-gray-400 dark:bg-gray-800 rounded-md cursor-pointer flex items-center" |
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.
nitpick: Minor hover state fix.
className="px-2 p-1 text-gray-500 bg-gray-100 dark:text-gray-400 dark:bg-gray-800 rounded-md cursor-pointer flex items-center" | |
className="px-2 p-1 text-gray-500 bg-gray-50 dark:text-gray-400 dark:bg-gray-800 rounded-md cursor-pointer flex items-center hover:bg-gray-100 dark:hover:bg-gray-700" |
customInput={<DateDisplay />} | ||
dateFormat={"MMM d, yyyy"} | ||
/> | ||
<h2 className="mx-2">to</h2> |
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.
nitpick: Minor spacing fix.
<h2 className="mx-2">to</h2> | |
<h2 className="mx-1">to</h2> |
@@ -160,13 +175,67 @@ function UsageView({ attributionId }: UsageViewProps) { | |||
|
|||
const headerTitle = attributionId.kind === "team" ? "Team Usage" : "Personal Usage"; | |||
|
|||
const DateDisplay = forwardRef((arg: any, ref: any) => ( | |||
<div | |||
className="px-2 p-1 text-gray-500 bg-gray-100 dark:text-gray-400 dark:bg-gray-800 rounded-md cursor-pointer flex items-center" |
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.
nitpick: Minor line height fix.
className="px-2 p-1 text-gray-500 bg-gray-100 dark:text-gray-400 dark:bg-gray-800 rounded-md cursor-pointer flex items-center" | |
className="px-2 py-0.5 text-gray-500 bg-gray-100 dark:text-gray-400 dark:bg-gray-800 rounded-md cursor-pointer flex items-center" |
</div> | ||
} | ||
subtitle={ | ||
<div className="tracking-wide flex mt-3 items-center"> |
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.
nitpick: Minor spacing issue, breaking line height.
<div className="tracking-wide flex mt-3 items-center"> | |
<div className="tracking-wide flex items-center"> |
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.
Removing this looks bad. Are you sure? If so please fix in a follow up.
</div> | ||
} | ||
subtitle={ | ||
<div className="tracking-wide flex mt-3 items-center"> |
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.
issue(non-blocking): This structure breaks slightly the heigh of the header but let's fix this in #6764 unless you'd like to move this element in the div
above, see subtitle and bottom border.
959e374
to
f7ef9a8
Compare
f7ef9a8
to
7f075ea
Compare
/unhold |
Description
This change allows to set custom date ranges on the usage view.
Related Issue(s)
See #14463, #13288
How to test
Release Notes
Documentation
Werft options:
If enabled this will build
install/preview
Valid options are
all
,workspace
,webapp
,ide