-
Notifications
You must be signed in to change notification settings - Fork 61
fix(ws): Updates to Table Columns, Expandable Rows, and Theming #432
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
fix(ws): Updates to Table Columns, Expandable Rows, and Theming #432
Conversation
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 PR @jenny-s51!
workspaces/frontend/src/app/types.ts
Outdated
@@ -7,6 +7,20 @@ import { | |||
Workspace, | |||
} from '~/shared/api/backendApiTypes'; | |||
|
|||
export interface WorkspacesColumnNames { |
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 interface is not being used anymore. Could you please remove it?
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.
Yes, nice catch here @caponetto
const jupyterLabVersion = imageConfig.labels.find( | ||
(label) => label.key === 'jupyterlabVersion', | ||
)?.value; | ||
const pythonVersion = imageConfig.labels.find((label) => label.key === 'pythonVersion')?.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.
I think we can define a map of label keys and their corresponding text that we want to extract, and then create a generic code to extract and render them down there.
Something like:
const PACKAGE_LABELS: Record<string, string> = {
jupyterlabVersion: 'JupyterLab',
pythonVersion: 'Python',
// Add more mappings here as needed later
};
...
const { labels } = workspace.podTemplate.options.imageConfig.current;
const renderedItems = Object.entries(PACKAGE_LABELS).flatMap(([key, label]) => {
const value = labels.find((l) => l.key === key)?.value;
return value ? <ListItem key={key}>{`${label} v${value}`}</ListItem> : [];
});
...
<List isPlain>{renderedItems}</List>
We'd probably also want to show something else if no matching labels are found, i.e., renderedItems.length === 0
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 @caponetto, great idea - this is much cleaner and more maintainable!
@@ -38,7 +40,6 @@ import { | |||
FilterableDataFieldKey, | |||
SortableDataFieldKey, | |||
} from '~/app/filterableDataHelper'; | |||
import { ExpandedWorkspaceRow } from '~/app/pages/Workspaces/ExpandedWorkspaceRow'; |
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.
As ExpandedWorkspaceRow
is not being used anymore, can we delete its file?
name: { label: 'Name', isFilterable: true, isSortable: true }, | ||
kind: { label: 'Kind', isFilterable: true, isSortable: true }, | ||
namespace: { label: 'Namespace', isFilterable: true, isSortable: true }, | ||
image: { label: 'Image', isFilterable: true, isSortable: true }, | ||
podConfig: { label: 'Pod Config', isFilterable: true, isSortable: true }, | ||
state: { label: 'State', isFilterable: true, isSortable: true }, | ||
homeVol: { label: 'Home Vol', isFilterable: true, isSortable: true }, |
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.
I think we should remove homeVol
from here as well, right?
<Tr isExpanded> | ||
<Td /> | ||
<Td dataLabel="Storage"> | ||
<ExpandableRowContent> | ||
<WorkspaceStorage workspace={workspace} /> | ||
</ExpandableRowContent> | ||
</Td> | ||
<Td> | ||
<ExpandableRowContent> | ||
<WorkspacePackageDetails workspace={workspace} /> | ||
</ExpandableRowContent> | ||
</Td> | ||
<Td> | ||
<ExpandableRowContent> | ||
<WorkspaceConfigDetails workspace={workspace} /> | ||
</ExpandableRowContent> | ||
</Td> | ||
<Td /> | ||
<Td /> | ||
<Td /> | ||
<Td /> | ||
</Tr> |
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.
Can we extract this piece of code to a dedicated component like it was being done with ExpandedWorkspaceRow
?
Also, we are assuming here 8 <Td>
but we should account for the scenario where we could have less columns (dictated by visibleColumnKeys
) due to the hiddenColumns
prop. All this logic could be calculated inside its own component. Saying this because the layout would break if we keep this code and show less columns.
55cb797
to
d654a5f
Compare
// Import the type from WorkspaceTable | ||
export type WorkspaceTableColumnKeys = | ||
| 'name' | ||
| 'kind' | ||
| 'namespace' | ||
| 'image' | ||
| 'state' | ||
| 'gpu' | ||
| 'idleGpu' | ||
| 'lastActivity' | ||
| 'connect' | ||
| 'actions'; |
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 type is not necessary, is it? We can keep using the WorkspaceTableColumnKeys
from '~/app/components/WorkspaceTable'
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.
Nice catch - fixed to import and apply the type from WorkspaceTable
.
{visibleColumnKeys.map((columnKey) => ( | ||
{canExpandRows && <Th width={10} screenReaderText="expand-action" />} | ||
{visibleColumnKeys.includes('name') && ( | ||
<Th |
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.
I think we can include the width information in the structure that defineDataFields
composes to avoid code duplication but that's an improvement we can do later in a future opportunity.
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.
Yes, great idea - I've added width
to DataFieldDefinition so the widths are no longer hardcoded here. Is this what you had in mind @caponetto ?
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.
Exactly! And also replace all the repeated <Th ...
definitions with
{visibleColumnKeys.map((columnKey) => (
<Th
width={wsTableColumns[columnKey].width}
key={`workspace-table-column-${columnKey}`}
sort={getSortParams(columnKey)}
aria-label={columnKey}
modifier="wrap"
screenReaderText={columnKey}
>
{wsTableColumns[columnKey].label}
</Th>
))}
WDYT?
@@ -32,18 +32,16 @@ describe('Application', () => { | |||
it('filter rows with multiple filters', () => { |
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.
I see these tests were meant to test multiple filters
but now they only add one single filter, which doesn't actually test the target scenarios. Since PodConfig filter no longer exists, can we use another one? State or image for instance.
2ff746a
to
9409445
Compare
/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.
Hi @jenny-s51 , thanks for the changes, it looks much better!
One thing I believe we could consider is making the width used by the Home volume
and Cluster storage
information a little bigger. Perhaps we could force the "lock" and "copy" icons to stay at the same line as the volume name and mount path even when the screen is smaller, wdyt?
/assign |
a8f0094
to
3dc1efc
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.
Thank you for your review @paulovmr , and nice catch - I've updated the layout and column width to prevent the icons from wrapping. Glad to handle any more changes in a follow-up PR so we can handle it outside of this scope - let me know if this is what you had in mind.
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 !
/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.
@jenny-s51 Just one thing, I think we are only missing the change about the repeated <Th ...
that @caponetto commented to be able to merge this PR.
46ab64c
to
ad51745
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.
Thank you @paulovmr , updated.
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
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.
.
ce736b1
to
684b48b
Compare
Signed-off-by: Jenny <[email protected]> add icon to workspaceKindsColumns interface fix(ws): Update table with expandable variant and fix styles fix secondary border in menu toggle fix menu toggle expanded text color and update icon to use status prop remove unused files add cluster storage description list group Signed-off-by: Jenny <[email protected]> Add title and packages revert form label styling, revert homeVol column fix linting fix lint Signed-off-by: Jenny <[email protected]> Add PR code suggestions, remove unused interfaces Signed-off-by: Jenny <[email protected]> remove unused import Signed-off-by: Jenny <[email protected]> fix filterWorkspacesTest Signed-off-by: Jenny <[email protected]> fix(ws): apply feedback to fix Cypress tests Signed-off-by: Jenny <[email protected]> Update tests, add width to defineDataFields, remove duplicate WorkspaceTableColumnKeys type Signed-off-by: Jenny <[email protected]> fix wrapping behavior Signed-off-by: Jenny <[email protected]> Replace Th values with mapped instance Signed-off-by: Jenny <[email protected]> revert column order Signed-off-by: Jenny <[email protected]> remove hardcoded package label instances Signed-off-by: Jenny <[email protected]> delete cursor rule
20973a2
to
7fa255c
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
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.
/approve
/lgtm
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: ederign The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
…flow#432) Signed-off-by: Jenny <[email protected]> add icon to workspaceKindsColumns interface fix(ws): Update table with expandable variant and fix styles fix secondary border in menu toggle fix menu toggle expanded text color and update icon to use status prop remove unused files add cluster storage description list group Signed-off-by: Jenny <[email protected]> Add title and packages revert form label styling, revert homeVol column fix linting fix lint Signed-off-by: Jenny <[email protected]> Add PR code suggestions, remove unused interfaces Signed-off-by: Jenny <[email protected]> remove unused import Signed-off-by: Jenny <[email protected]> fix filterWorkspacesTest Signed-off-by: Jenny <[email protected]> fix(ws): apply feedback to fix Cypress tests Signed-off-by: Jenny <[email protected]> Update tests, add width to defineDataFields, remove duplicate WorkspaceTableColumnKeys type Signed-off-by: Jenny <[email protected]> fix wrapping behavior Signed-off-by: Jenny <[email protected]> Replace Th values with mapped instance Signed-off-by: Jenny <[email protected]> revert column order Signed-off-by: Jenny <[email protected]> remove hardcoded package label instances Signed-off-by: Jenny <[email protected]> delete cursor rule
Closes #372
Overview
Table Updates
Theming & Visual Fixes (MUI-theme.scss)
--mui-button--BorderWidth: 1px
and improved split button border color, style, and consistency.Before:

After:
