- Notifications
You must be signed in to change notification settings - Fork920
feat: improve metrics and UI for user engagement on the platform#16134
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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
Uh oh!
There was an error while loading.Please reload this page.
Conversation
github-actionsbot commentedJan 15, 2025 • edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
✔️ PR 16134 Updated successfully. |
I do not see the chart for License utilization as per thisdesign. Has that been removed from Scope? Or Is that coming in a separate PR? |
site/tailwind.config.js Outdated
| @@ -16,6 +16,7 @@ module.exports = { | |||
| fontSize: { | |||
| "2xs": ["0.625rem", "0.875rem"], | |||
| sm: ["0.875rem", "1.5rem"], | |||
| md: ["1rem", "1.5rem"], | |||
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.
text-base covers the size ["1rem", "1.5rem"] so there shouldn't be a need to explicitly set md, seehttps://tailwindcss.com/docs/font-size#setting-the-line-height
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.
Sorry to have to block this. As identified by my comments, there are some changes in here that should not make it into main. See@matifali's comment above as well.
site/src/pages/DeploymentSettingsPage/GeneralSettingsPage/GeneralSettingsPage.tsx OutdatedShow resolvedHide resolved
Uh oh!
There was an error while loading.Please reload this page.
site/src/pages/DeploymentSettingsPage/GeneralSettingsPage/GeneralSettingsPageView.tsxShow resolvedHide resolved
Uh oh!
There was an error while loading.Please reload this page.
The PR has been updated but is still waiting on a few definitions:
Despite these pending decisions, it’s ready for review. |
| className="size-3 inline-flex items-center justify-center" | ||
| aria-label="Legend for license seat limit in the chart" | ||
| > | ||
| <div className="w-full border-b-1 border-t-1 border-dashed border-content-disabled" /> |
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.
| <divclassName="w-full border-b-1 border-t-1 border-dashed border-content-disabled"/> | |
| <divclassName="w-full border-b border-t border-dashed border-content-disabled"/> |
this reduces the width to better match the design
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.
From a frontend perspective looks good overall 👍
Great work@BrunoQuaresma. A few small notes for consideration, but we can get to them as follow-up:
|
3217cb8 intomainUh oh!
There was an error while loading.Please reload this page.
Uh oh!
There was an error while loading.Please reload this page.
You can explore the new chart onStorybook.
Preview:
Close#15297