-
Notifications
You must be signed in to change notification settings - Fork 53
improve: UX Enhancements in workspace summary #473
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
base: notebooks-v2
Are you sure you want to change the base?
improve: UX Enhancements in workspace summary #473
Conversation
Signed-off-by: DominikKawka <[email protected]>
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: The full list of commands accepted by this bot can be found here.
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
Signed-off-by: DominikKawka <[email protected]>
Hi @jenny-s51 , could you take a look at these changes please? /ok-to-test |
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.
Thank you for making these changes and great work here @dominikkawka ! Just one comment below - I put up a PR to support PF utility classes. Once it's merged you can consume the changes and apply the code suggestions so we follow best practices for Patternfly component styling.
#476 cc @paulovmr , if you could PTAL when you have a moment it would be much appreciated so we can unblock this. Thanks a lot here 🚀
@@ -65,7 +65,9 @@ const WorkspaceKindSummaryExpandableCard: React.FC<WorkspaceKindSummaryExpandabl | |||
<Flex wrap="wrap"> | |||
<SectionFlex title="Total GPUs in use"> | |||
<FlexItem> | |||
<Content> | |||
<Content | |||
style={{ fontSize: LargeFontSize.value, fontWeight: BoldFontWeight.value }} |
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.
When #476 is merged, we can apply the corresponding utility classes here:
style={{ fontSize: LargeFontSize.value, fontWeight: BoldFontWeight.value }} | |
className="pf-v6-u-font-size-4xl pf-v6-u-font-weight-bold" |
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 @jenny-s51 , #476 is now merged.
Signed-off-by: DominikKawka <[email protected]>
Signed-off-by: DominikKawka <[email protected]>
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.
Enhancements LGTM @dominikkawka!
Can we get your signoff here as well @tobiastal? Just a quick question for you on the Idle GPU Workspaces too. Currently it looks like this:

For consistency should we update this to be left aligned?
Hey @jenny-s51 ![]() |
Signed-off-by: Jenny <[email protected]>
Thank you for your review @tobiastal - yes, great catch! @dominikkawka I've PR-ed your branch with the changes: dominikkawka#1 Can you rebase your branch with the latest and then pull in the changes? Then it should be all set to merge. 🚀 |
Signed-off-by: Dominik Kawka <[email protected]>
fix(ws): apply left alignment and fix padding to match other items
Signed-off-by: DominikKawka <[email protected]>
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 the updates @dominikkawka - LGTM.
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
Hi @ederign, when you have a moment, can you please kindly take a look? If you can help us merge this one would be much appreciated. Thank you 🚀 |
closes: #461, #462, #463,
related: #458