feat(ws): Add popup to Workspace data vol column in workspaces table #160

ElayAharoni · 2024-12-12T15:14:46Z

Fixes: #149

added popup with the worspace data vol info, that will show when hovering on the data vol column field.

please inform me if anything needs to be changes in matter of functionality or styling.

added a picture of the change:

paulovmr

Hi @ElayAharoni , thanks for the PR! For this task, I asked @jenny-s51 from UX to give her input on the best component to use to show those data volumes. One thing that would need to change, is that we need to show not only the first but all elements from the workspace.podTemplate.volumes.data array, so there might be a need for an alternative component than the Popover.

ElayAharoni · 2024-12-16T22:18:22Z

Hi Paulo, thanks for your comment.

I think that if we need to show larger data than what is shown right now, then maybe it will be better not to show it when hovering on the field.
But to show it when clicking on the helpIcon that next to it, and then we can open a dialog or other component with all the relevant data.

Please let me know what you think about it.

paulovmr · 2024-12-17T14:09:28Z

Hi @jenny-s51 , here is the PR about the data volumes column.

jenny-s51 · 2024-12-17T14:53:13Z

Hi @paulovmr , thank you for tagging me! I see #150 is a similar ticket as well.

Per our recent conversation this morning we agreed that the expandable table variant is an effective approach and component to use for these cases - since we want to show additional data for multiple columns in this table view, using the PF expandable table variant offers more flexibility and provides a visual cue to the user that there is more data within the table view. WDYT @ElayAharoni ?

ElayAharoni · 2024-12-17T15:23:51Z

I think it will be the best approach, as it is a solution for all the columns that need to show additional data.
And it is also the way it is implemented in the ODH.

Do you want me to implement it in my pr and then all the other relevant PR's will be based on mine?
@paulovmr , @jenny-s51

jenny-s51 · 2024-12-17T15:43:56Z

Yes, that sounds like a good plan, @ElayAharoni.

ElayAharoni · 2024-12-18T09:31:16Z

hi @paulovmr , @jenny-s51.
i have made the changes, to show the data via expendable table row.
attached screenshot for reference:

paulovmr · 2024-12-18T17:13:53Z

IMO the idea of adding a expandable row would eliminate the need for a normal column for that field, since the expandable part would be presented as a separate row that adds more information, and not an extension to the columns, like seen in here. @jenny-s51 do you agree?

jenny-s51

This is a good callout @paulovmr - if we are showing additional data for multiple columns, the data should be presented within the span of the column it is associated with.

Here is a good example of how this is currently done in the RHOAI Dashboard, for example the "Container size" column has Limits and Memory within the same column span (see NotebookTableRow.tsx).

We also want to make sure to wrap the content of each child row cell in ExpandableRowContent to apply the correct styles. @ElayAharoni

ElayAharoni · 2024-12-19T11:08:29Z

hi @jenny-s51 @paulovmr.
i have made the requested fixes.
attached new snapshot:

ElayAharoni · 2024-12-22T08:45:30Z

hi @paulovmr , @jenny-s51.
after the discussions we had this is the current fix.
please review when you have time.
attached latest snapshot:

paulovmr · 2025-01-09T13:51:54Z

workspaces/frontend/src/app/pages/Workspaces/ExpandedDataVol.tsx

+  const workspaceDataVol = workspace.podTemplate.volumes.data;
+  return (
+    <>
+      <Content component="h3">Data Volumes:</Content>


@ElayAharoni IMO the List component from PatternFly might be a better fit for this kind of information on the UI. Please consider replacing the Content usage for the List component.

paulovmr · 2025-01-09T13:53:40Z

workspaces/frontend/src/app/pages/Workspaces/ExpandedDataVol.tsx

+      <Content component="h3">Data Volumes:</Content>
+      {workspaceDataVol.map((data, index) => (
+        <Content key={`data-vol-${index}`} component="blockquote">
+          <div>


In general, it is best to avoid the usage of plain HTML components in the middle of PatternFly components to avoid breaking future CSS rules.

paulovmr · 2025-01-09T13:55:41Z

workspaces/frontend/src/app/pages/Workspaces/ExpandedDataVol.tsx

+  workspace: Workspace;
+}
+
+export const ExpandedDataVol: React.FC<ExpandedDataVolProps> = ({ workspace }) => {


Please consider renaming this component (and also other associated declarations).

Suggested change

export const ExpandedDataVol: React.FC<ExpandedDataVolProps> = ({ workspace }) => {

export const DataVolumesList: React.FC<ExpandedDataVolProps> = ({ workspace }) => {

ElayAharoni · 2025-01-09T14:01:47Z

@paulovmr thanks for your comments i will fix them

paulovmr

Hi @ElayAharoni , left a few minor changes inline. One thing that is important as well is to remove the Data Vol column from the table, now that the information has moved to the expandable row. Also please add the new type WorkspacesColumnNames to the variable columnNames.

paulovmr · 2025-01-09T16:03:18Z

workspaces/frontend/src/app/pages/Workspaces/DataVolumesList.tsx

+import * as React from 'react';
+import { Workspace } from '~/shared/types';
+
+interface ExpandedDataVolProps {


Suggested change

interface ExpandedDataVolProps {

interface DataVolumesListProps {

paulovmr · 2025-01-09T16:03:37Z

workspaces/frontend/src/app/pages/Workspaces/DataVolumesList.tsx

+  workspace: Workspace;
+}
+
+export const DataVolumesList: React.FC<ExpandedDataVolProps> = ({ workspace }) => {


Suggested change

export const DataVolumesList: React.FC<ExpandedDataVolProps> = ({ workspace }) => {

export const DataVolumesList: React.FC<DataVolumesListProps> = ({ workspace }) => {

paulovmr · 2025-01-09T17:53:30Z

workspaces/frontend/src/app/pages/Workspaces/Workspaces.tsx

              {columnNames.cpu}
            </Th>
            <Th sort={getSortParams(8)} info={{ tooltip: 'Workspace memory usage' }}>
              {columnNames.ram}
            </Th>
            <Th sort={getSortParams(9)}>{columnNames.lastActivity}</Th>
+            <Th sort={getSortParams(6)}>{columnNames.lastActivity}</Th>


I believe there was an issue with the conflict resolution here, because the lastActivity column is duplicated. Also, because the Data Vol column was removed, it has to also be removed from the sortable rows, and the parameters to the getSortParams should be changed too.

yeah you are right, i have fixed and pused for the final time i hope

Signed-off-by: Elay Aharoni (EXT-Nokia) <[email protected]>

google-oss-prow · 2025-01-13T13:02:11Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: ederign, paulovmr

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:

~~workspaces/frontend/OWNERS~~ [ederign]

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

ederign · 2025-01-13T13:02:11Z

/lgtm

google-oss-prow bot requested review from ederign and thesuperzapper December 12, 2024 15:14

google-oss-prow bot added the size/M label Dec 12, 2024

ElayAharoni force-pushed the Workspace-Data-Vol-column branch from 61b14b3 to a909536 Compare December 15, 2024 08:35

paulovmr reviewed Dec 16, 2024

View reviewed changes

ElayAharoni force-pushed the Workspace-Data-Vol-column branch from a909536 to dd73111 Compare December 18, 2024 09:27

ElayAharoni force-pushed the Workspace-Data-Vol-column branch from dd73111 to fdaa028 Compare December 18, 2024 09:47

jenny-s51 reviewed Dec 18, 2024

View reviewed changes

ElayAharoni force-pushed the Workspace-Data-Vol-column branch from fdaa028 to 0164324 Compare December 19, 2024 11:07

ElayAharoni force-pushed the Workspace-Data-Vol-column branch from 0164324 to c81c44e Compare December 22, 2024 08:43

ElayAharoni force-pushed the Workspace-Data-Vol-column branch 2 times, most recently from bbc1bc3 to 3b67e91 Compare January 8, 2025 18:01

google-oss-prow bot added size/L and removed size/M labels Jan 8, 2025

ElayAharoni force-pushed the Workspace-Data-Vol-column branch from 3b67e91 to 7f3288c Compare January 9, 2025 13:38

paulovmr suggested changes Jan 9, 2025

View reviewed changes

ElayAharoni force-pushed the Workspace-Data-Vol-column branch from 7f3288c to d991379 Compare January 9, 2025 15:01

paulovmr suggested changes Jan 9, 2025

View reviewed changes

ElayAharoni force-pushed the Workspace-Data-Vol-column branch from d991379 to 037519c Compare January 9, 2025 16:56

paulovmr suggested changes Jan 9, 2025

View reviewed changes

Workspace Data Vol column popup add

8fcab6c

Signed-off-by: Elay Aharoni (EXT-Nokia) <[email protected]>

ElayAharoni force-pushed the Workspace-Data-Vol-column branch from 037519c to 8fcab6c Compare January 12, 2025 08:58

paulovmr approved these changes Jan 13, 2025

View reviewed changes

ederign approved these changes Jan 13, 2025

View reviewed changes

google-oss-prow bot added the approved label Jan 13, 2025

google-oss-prow bot assigned ederign Jan 13, 2025

google-oss-prow bot added the lgtm label Jan 13, 2025

google-oss-prow bot merged commit 249a456 into kubeflow:notebooks-v2 Jan 13, 2025
4 checks passed

thesuperzapper mentioned this pull request Jan 16, 2025

Add @paulovmr as member kubeflow/internal-acls#734

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(ws): Add popup to Workspace data vol column in workspaces table #160

feat(ws): Add popup to Workspace data vol column in workspaces table #160

ElayAharoni commented Dec 12, 2024

paulovmr left a comment •

edited

Loading

ElayAharoni commented Dec 16, 2024

paulovmr commented Dec 17, 2024

jenny-s51 commented Dec 17, 2024 •

edited

Loading

ElayAharoni commented Dec 17, 2024 •

edited

Loading

jenny-s51 commented Dec 17, 2024

ElayAharoni commented Dec 18, 2024 •

edited

Loading

paulovmr commented Dec 18, 2024

jenny-s51 left a comment

ElayAharoni commented Dec 19, 2024

ElayAharoni commented Dec 22, 2024

paulovmr Jan 9, 2025

paulovmr Jan 9, 2025

paulovmr Jan 9, 2025

ElayAharoni commented Jan 9, 2025

paulovmr left a comment

paulovmr Jan 9, 2025

paulovmr Jan 9, 2025

paulovmr Jan 9, 2025 •

edited

Loading

ElayAharoni Jan 12, 2025

google-oss-prow bot commented Jan 13, 2025

ederign commented Jan 13, 2025

	export const ExpandedDataVol: React.FC<ExpandedDataVolProps> = ({ workspace }) => {
	export const DataVolumesList: React.FC<ExpandedDataVolProps> = ({ workspace }) => {

	interface ExpandedDataVolProps {
	interface DataVolumesListProps {

	export const DataVolumesList: React.FC<ExpandedDataVolProps> = ({ workspace }) => {
	export const DataVolumesList: React.FC<DataVolumesListProps> = ({ workspace }) => {

feat(ws): Add popup to Workspace data vol column in workspaces table #160

feat(ws): Add popup to Workspace data vol column in workspaces table #160

Conversation

ElayAharoni commented Dec 12, 2024

paulovmr left a comment • edited Loading

Choose a reason for hiding this comment

ElayAharoni commented Dec 16, 2024

paulovmr commented Dec 17, 2024

jenny-s51 commented Dec 17, 2024 • edited Loading

ElayAharoni commented Dec 17, 2024 • edited Loading

jenny-s51 commented Dec 17, 2024

ElayAharoni commented Dec 18, 2024 • edited Loading

paulovmr commented Dec 18, 2024

jenny-s51 left a comment

Choose a reason for hiding this comment

ElayAharoni commented Dec 19, 2024

ElayAharoni commented Dec 22, 2024

paulovmr Jan 9, 2025

Choose a reason for hiding this comment

paulovmr Jan 9, 2025

Choose a reason for hiding this comment

paulovmr Jan 9, 2025

Choose a reason for hiding this comment

ElayAharoni commented Jan 9, 2025

paulovmr left a comment

Choose a reason for hiding this comment

paulovmr Jan 9, 2025

Choose a reason for hiding this comment

paulovmr Jan 9, 2025

Choose a reason for hiding this comment

paulovmr Jan 9, 2025 • edited Loading

Choose a reason for hiding this comment

ElayAharoni Jan 12, 2025

Choose a reason for hiding this comment

google-oss-prow bot commented Jan 13, 2025

ederign commented Jan 13, 2025

paulovmr left a comment •

edited

Loading

jenny-s51 commented Dec 17, 2024 •

edited

Loading

ElayAharoni commented Dec 17, 2024 •

edited

Loading

ElayAharoni commented Dec 18, 2024 •

edited

Loading

paulovmr Jan 9, 2025 •

edited

Loading