Projects list view: expand workbenches column to a table #3207

jeff-phillips-18 · 2024-09-13T12:15:21Z

Description

Update the projects list view from having nested subcolumns to show workbenches to using an expandable column to show an expanded view with a table listing the workbenches for the project.

How Has This Been Tested?

On a system with multiple projects, navigate to the Data Science Projects page.
Notice that workbenches are now indicated as an icon (wrench) and a count.
Click on the icon/count for a project with existing workbenches.
See the row expand to show a table of the workbenches belonging to the project.

Manually tested

Test Impact

Updated the e2e tests

Screen shot

Request review criteria:

Self checklist (all need to be checked):

The developer has manually tested the changes and verified that the changes work
Testing instructions have been added in the PR body (for PRs involving changes that are not immediately obvious).
The developer has added tests or explained why testing cannot be added (unit or cypress tests for related changes)

If you have UI changes:

Included any necessary screenshots or gifs if it was a UI change.
Included tags to the UX team if it was a UI/UX change.

/cc @tobiastal

openshift-ci · 2024-09-13T12:15:26Z

@jeff-phillips-18: GitHub didn't allow me to request PR reviews from the following users: tobiastal.

Note that only opendatahub-io members and repo collaborators can review this PR, and authors cannot review their own PRs.

In response to this:

Closes RHOAIENG-10494

Description

Update the projects list view from having nested subcolumns to show workbenches to using an expandable column to show an expanded view with a table listing the workbenches for the project.

How Has This Been Tested?

On a system with multiple projects, navigate to the Data Science Projects page.
Notice that workbenches are now indicated as an icon (wrench) and a count.
Click on the icon/count for a project with existing workbenches.
See the row expand to show a table of the workbenches belonging to the project.

Manually tested

Test Impact

Updated the e2e tests

Screen shot

Request review criteria:

Self checklist (all need to be checked):

The developer has manually tested the changes and verified that the changes work

Testing instructions have been added in the PR body (for PRs involving changes that are not immediately obvious).

The developer has added tests or explained why testing cannot be added (unit or cypress tests for related changes)

If you have UI changes:

Included any necessary screenshots or gifs if it was a UI change.

Included tags to the UX team if it was a UI/UX change.

/cc @tobiastal

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository.

jeff-phillips-18 · 2024-09-16T17:37:21Z

/retest

codecov · 2024-09-16T17:48:58Z

Codecov Report

Attention: Patch coverage is 79.72028% with 29 lines in your changes missing coverage. Please review.

Project coverage is 85.28%. Comparing base (b5351a7) to head (edc6b62).
Report is 6 commits behind head on main.

Files with missing lines	Patch %	Lines
...rc/pages/projects/notebook/NotebookStateStatus.tsx	70.73%	12 Missing ⚠️
...es/projects/screens/projects/notebookTableData.tsx	28.57%	10 Missing ⚠️
.../screens/projects/ProjectTableRowNotebookTable.tsx	71.42%	4 Missing ⚠️
...ages/projects/screens/projects/ProjectTableRow.tsx	84.61%	2 Missing ⚠️
.../pages/projects/notebook/NotebookActionsColumn.tsx	97.05%	1 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #3207      +/-   ##
==========================================
- Coverage   85.39%   85.28%   -0.11%     
==========================================
  Files        1277     1281       +4     
  Lines       28082    28120      +38     
  Branches     7487     7484       -3     
==========================================
+ Hits        23980    23983       +3     
- Misses       4102     4137      +35

Files with missing lines	Coverage Δ
frontend/src/api/k8s/notebooks.ts	`97.45% <100.00%> (ø)`
frontend/src/images/icons/NotebookIcon.ts	`100.00% <100.00%> (ø)`
...ects/screens/detail/notebooks/NotebookTableRow.tsx	`84.21% <100.00%> (-0.24%)`	⬇️
...ages/projects/screens/projects/ProjectListView.tsx	`75.00% <100.00%> (+2.27%)`	⬆️
...reens/projects/ProjectTableRowNotebookTableRow.tsx	`100.00% <100.00%> (ø)`
.../src/pages/projects/screens/projects/tableData.tsx	`100.00% <ø> (ø)`
...rontend/src/utilities/useWatchProjectNotebooks.tsx	`100.00% <100.00%> (ø)`
.../pages/projects/notebook/NotebookActionsColumn.tsx	`93.75% <97.05%> (ø)`
...ages/projects/screens/projects/ProjectTableRow.tsx	`90.90% <84.61%> (+4.79%)`	⬆️
.../screens/projects/ProjectTableRowNotebookTable.tsx	`71.42% <71.42%> (ø)`
... and 2 more

... and 10 files with indirect coverage changes

Continue to review full report in Codecov by Sentry.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update b5351a7...edc6b62. Read the comment docs.

jenny-s51

These changes are looking good @jeff-phillips-18 . Tested in the UI against the mocks. Left a comment below.

jenny-s51 · 2024-09-17T21:11:08Z

frontend/src/images/icons/NotebookIcon.ts

@@ -0,0 +1,13 @@
+import { createIcon } from '@patternfly/react-icons/dist/esm/createIcon';
+
+const NotebookIcon = createIcon({


According to the mocks, it looks like this icon should be solid. Wondering if we can update the svg path here? WDYT @jeff-phillips-18

@jenny-s51 You're looking at the old mocks, In the updatedmocks (which are ready for dev), we're using the outlined icon, just like Jeff added.

One small difference is that the icon appears slightly smaller than it should be (compared to the mocks).
@jeff-phillips-18 Could you take a look at this?

The size in the table is based on the font size (14px) while the mocks show 16px. I have updated the code to set the icons size to 16px. Screen shot above has been updated.

tobiastal · 2024-09-19T12:57:47Z

LGTM

jenny-s51

LGTM, retested locally and compared against updated mocks. Icon size update looks good.

openshift-ci · 2024-09-19T13:45:09Z

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: jenny-s51
Once this PR has been reviewed and has the lgtm label, please assign manosnoam for approval. For more information see the Kubernetes Code Review Process.

The full list of commands accepted by this bot can be found here.

Needs approval from an approver in each of these files:

frontend/OWNERS

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

andrewballantyne · 2024-09-19T16:10:37Z

Does the stopped state have a pause icon? It's not paused... there is no data persistence in memory

jeff-phillips-18 · 2024-09-19T18:08:39Z

Does the stopped state have a pause icon? It's not paused... there is no data persistence in memory

@tobiastal Mocks show a paused icon. Is this what we want?

openshift-ci · 2024-09-20T12:55:08Z

New changes are detected. LGTM label has been removed.

jeff-phillips-18 · 2024-09-20T12:55:57Z

Got an updated icon for the workbench in the row:

christianvogt · 2024-09-20T14:49:51Z

frontend/src/pages/projects/screens/projects/ProjectTableRow.tsx

+      <Tr isExpanded={!!expandColumn}>
+        <Td
+          dataLabel="workbenches"
+          colSpan={4}
+          style={{
+            borderTopWidth: 1,
+            borderTopStyle: 'solid',
+            borderTopColor: 'var(--pf-v5-global--BorderColor--100)',
+          }}
+        >
+          <ProjectTableRowNotebookTable obj={project} />
+        </Td>
+      </Tr>


This row is rendered by PF no matter if it's expanded or not.
This is causing extra network requests to be made.
Add conditional rendering.

christianvogt · 2024-09-20T15:03:48Z

frontend/src/pages/projects/notebook/NotebookActionsColumn.tsx

+          {
+            isDisabled: isStarting || isStopping,
+            title: 'Edit workbench',
+            onClick: () => {
+              navigate(
+                `/projects/${project.metadata.name}/spawner/${notebookState.notebook.metadata.name}`,
+              );
+            },
+          },


If I cancel out of editing the workbench, I don't end up back at the project list view.
If I press the browser back button, I end up on the project list view however I am on the wrong page or the expanded section is no longer open.

Updated the spawner paged to navigate back on a cancel

christianvogt · 2024-09-20T15:06:31Z

frontend/src/pages/projects/notebook/NotebookStateStatus.tsx

+        icon: <InProgressIcon style={{ animation: rotate }} />,
+      };
+    }
+    if (isStopping) {
+      return {
+        label: 'Stopping',
+        color: 'grey',
+        icon: <SyncAltIcon style={{ animation: rotate }} />,


We use these icons in several places.

PF doesn't have a means to spin icons? If not then, make then reusable components with their custom animation so we can update those other locations at a later date.

christianvogt · 2024-09-20T15:24:44Z

frontend/src/api/k8s/notebooks.ts

@@ -206,7 +206,7 @@ export const getStopPatch = (): Patch => ({
  value: getStopPatchDataString(),
 });

-export const getNotebooks = (namespace: string): Promise<NotebookKind[]> =>
+export const getNotebooks = (namespace?: string): Promise<NotebookKind[]> =>


Why this change?

oops... reverted

christianvogt · 2024-09-20T15:27:26Z

frontend/src/pages/projects/screens/projects/ProjectTableRowNotebookTable.tsx

+          <DeleteNotebookModal
+            notebook={notebookToDelete}
+            onClose={(deleted) => {
+              if (deleted) {
+                refresh();
+              }
+              setNotebookToDelete(undefined);
+            }}
+          />


Modals should be conditionally rendered.

christianvogt · 2024-09-20T15:29:39Z

frontend/src/pages/projects/screens/projects/ProjectTableRowNotebookTableRow.tsx

+  );
+  return (
+    <Tr style={{ border: 'none' }} data-testid="project-notebooks-table-row">
+      <Td dataLabel="workbenchName" style={{ paddingLeft: 'var(--pf-v5-global--spacer--sm)' }}>


cell value is misaligned with header:

jeff-phillips-18 requested a review from christianvogt September 13, 2024 12:15

jeff-phillips-18 force-pushed the projects-workbenches branch from 998f741 to db5d2ec Compare September 16, 2024 15:44

jenny-s51 self-requested a review September 17, 2024 15:10

jenny-s51 reviewed Sep 17, 2024

View reviewed changes

jeff-phillips-18 force-pushed the projects-workbenches branch 2 times, most recently from f2e8d01 to edc6b62 Compare September 18, 2024 13:44

jenny-s51 approved these changes Sep 19, 2024

View reviewed changes

openshift-ci bot assigned jenny-s51 Sep 19, 2024

openshift-ci bot added the lgtm label Sep 19, 2024

jeff-phillips-18 force-pushed the projects-workbenches branch from edc6b62 to 1999b53 Compare September 20, 2024 12:55

openshift-ci bot removed the lgtm label Sep 20, 2024

jeff-phillips-18 force-pushed the projects-workbenches branch 2 times, most recently from a9c48bd to 430e3a4 Compare September 20, 2024 13:06

christianvogt reviewed Sep 20, 2024

View reviewed changes

Projects list view: expand workbenches column to a table

8587d6b

jeff-phillips-18 force-pushed the projects-workbenches branch from 430e3a4 to 8587d6b Compare September 20, 2024 18:09

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Projects list view: expand workbenches column to a table #3207

Projects list view: expand workbenches column to a table #3207

jeff-phillips-18 commented Sep 13, 2024 •

edited

Loading

openshift-ci bot commented Sep 13, 2024

Description

How Has This Been Tested?

Test Impact

Screen shot

Request review criteria:

jeff-phillips-18 commented Sep 16, 2024

codecov bot commented Sep 16, 2024 •

edited

Loading

jenny-s51 left a comment

jenny-s51 Sep 17, 2024

tobiastal Sep 18, 2024

jeff-phillips-18 Sep 18, 2024

tobiastal commented Sep 19, 2024

jenny-s51 left a comment

openshift-ci bot commented Sep 19, 2024

andrewballantyne commented Sep 19, 2024

jeff-phillips-18 commented Sep 19, 2024

openshift-ci bot commented Sep 20, 2024

jeff-phillips-18 commented Sep 20, 2024

christianvogt Sep 20, 2024

christianvogt Sep 20, 2024

jeff-phillips-18 Sep 20, 2024

christianvogt Sep 20, 2024

christianvogt Sep 20, 2024

jeff-phillips-18 Sep 20, 2024

christianvogt Sep 20, 2024

christianvogt Sep 20, 2024

		@@ -0,0 +1,13 @@
		import { createIcon } from '@patternfly/react-icons/dist/esm/createIcon';

		const NotebookIcon = createIcon({

Projects list view: expand workbenches column to a table #3207

Are you sure you want to change the base?

Projects list view: expand workbenches column to a table #3207

Conversation

jeff-phillips-18 commented Sep 13, 2024 • edited Loading

Description

How Has This Been Tested?

Test Impact

Screen shot

Request review criteria:

openshift-ci bot commented Sep 13, 2024

Description

How Has This Been Tested?

Test Impact

Screen shot

Request review criteria:

jeff-phillips-18 commented Sep 16, 2024

codecov bot commented Sep 16, 2024 • edited Loading

Codecov Report

jenny-s51 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tobiastal commented Sep 19, 2024

jenny-s51 left a comment

Choose a reason for hiding this comment

openshift-ci bot commented Sep 19, 2024

andrewballantyne commented Sep 19, 2024

jeff-phillips-18 commented Sep 19, 2024

openshift-ci bot commented Sep 20, 2024

jeff-phillips-18 commented Sep 20, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jeff-phillips-18 commented Sep 13, 2024 •

edited

Loading

codecov bot commented Sep 16, 2024 •

edited

Loading