-
Notifications
You must be signed in to change notification settings - Fork 69
feat(ws): Notebooks 2.0 // Frontend // Workspace Kind Viewer // Live mockup #203 #237
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
feat(ws): Notebooks 2.0 // Frontend // Workspace Kind Viewer // Live mockup #203 #237
Conversation
paulovmr
left a comment
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 @liavweiss , thanks for the PR! I left a few comments inline. Also, as discussed in Slack, IMO this table should not have expandable rows, only the drawer for details.
| path: '/', | ||
| }, | ||
| { | ||
| label: 'WorkspaceKinds', |
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.
| label: 'WorkspaceKinds', | |
| label: 'Workspace Kinds', |
| onClick: () => viewDetailsClick(workspaceKind), | ||
| }, | ||
| { | ||
| isSeparator: 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.
No need for a separator here.
| const [expandedWorkspaceKindsNames, setExpandedWorkspaceKindsNames] = React.useState<string[]>( | ||
| [], | ||
| ); | ||
| const [selectedWorkspaceKind, setSelectedWorkspacekind] = React.useState<WorkspaceKind | null>( |
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.
| const [selectedWorkspaceKind, setSelectedWorkspacekind] = React.useState<WorkspaceKind | null>( | |
| const [selectedWorkspaceKind, setSelectedWorkspaceKind] = React.useState<WorkspaceKind | null>( |
| numberOfWorkspaces: 'Number Of Workspaces', | ||
| }; | ||
|
|
||
| const filterableColumns = { |
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.
Please consider adding the Description column here.
| {workspaceKind.deprecated ? ( | ||
| <Label color="red">Deprecated</Label> | ||
| ) : ( | ||
| <Tooltip content={workspaceKind.deprecationMessage}> |
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.
The Tooltip should be used when deprecated is true and not used when it is false.
|
|
||
| const filterableColumns = { | ||
| name: 'Name', | ||
| deprecated: 'Status', |
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.
For status filtering, I believe we should have the options listed, instead of a text input. Similarly to the "Attribute Search" PatternFly example.
…mockup kubeflow#203 Signed-off-by: Liav Weiss (EXT-Nokia) <liav.weiss.ext@nokia.com>
…mockup kubeflow#203 Signed-off-by: Liav Weiss (EXT-Nokia) <liav.weiss.ext@nokia.com>
…mockup kubeflow#203 Signed-off-by: Liav Weiss (EXT-Nokia) <liav.weiss.ext@nokia.com>
a60d2b2 to
3601fb8
Compare
paulovmr
left a comment
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 @liavweiss , I've commented just a few minor changes. Also, please consider adding useCallbackand useMemo hooks to the variables across the file. Thanks!
| const [activeSortDirection, setActiveSortDirection] = React.useState<'asc' | 'desc' | null>(null); | ||
|
|
||
| const getSortableRowValues = (workspaceKind: WorkspaceKind): (string | boolean | number)[] => { | ||
| const { icon, name, description, deprecated, numOfWrokspaces } = { |
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.
| const { icon, name, description, deprecated, numOfWrokspaces } = { | |
| const { icon, name, description, deprecated, numberOfWorkspaces } = { |
| name: 'Name', | ||
| description: 'Description', | ||
| deprecated: 'Status', | ||
| numberOfWorkspaces: 'Number Of Workspaces', |
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.
| numberOfWorkspaces: 'Number Of Workspaces', | |
| numberOfWorkspaces: 'Number of workspaces', |
jenny-s51
left a comment
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 work on this @liavweiss - left a comment below.
| <Th screenReaderText="Primary action" /> | ||
| </Tr> | ||
| </Thead> | ||
| {filteredWorkspaceKinds.map((workspaceKind, rowIndex) => ( |
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.
While testing the status filters, I noticed we’re not currently handling the empty state when the filtered list has a length of 0.
Could we add a check on the filtered results and render the empty state table in that case? WDYT @liavweiss ?
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.
emptyState.mp4
…mockup kubeflow#203 Signed-off-by: Liav Weiss (EXT-Nokia) <liav.weiss.ext@nokia.com>
paulovmr
left a comment
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 @liavweiss . There are a few other variables, such as event handlers and UI elements, that are missing useCallback and useMemo hooks, could you please add to them as well? Thanks!
| setIsStatusMenuOpen(!isStatusMenuOpen); | ||
| }; | ||
|
|
||
| function onStatusSelect( |
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.
| function onStatusSelect( | |
| const onStatusSelect = ( |
| function onStatusSelect( | ||
| event: React.MouseEvent | undefined, | ||
| itemId: string | number | undefined, | ||
| ) { |
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.
| ) { | |
| ) => { |
…mockup kubeflow#203 Signed-off-by: Liav Weiss (EXT-Nokia) <liav.weiss.ext@nokia.com>
…mockup kubeflow#203 Signed-off-by: Liav Weiss (EXT-Nokia) <liav.weiss.ext@nokia.com>
|
/ok-to-test |
paulovmr
left a comment
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
jenny-s51
left a comment
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.
Empty state looks great - thanks for the fix @liavweiss ! 🙌 We should also surface the empty state in the Workspaces table for consistency.
I’ll file a couple of follow-up issues:
- Empty state for Workspaces table - When filters return 0 results, we should show an empty state (same as the Workspace Kind viewer in this PR). @liavweiss would you be able to add this in a follow-up PR?
- Chip label padding - Needs a small tweak to align properly. I’ll create and assign this one to myself and get a fix in soon.
/lgtm
jenny-s51
left a comment
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.
Left one more comment @liavweiss - otherwise LGTM!
| { | ||
| label: 'Workspaces', | ||
| path: '/', | ||
| }, |
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.
| { | |
| label: 'Workspaces', | |
| path: '/', | |
| }, | |
| { | |
| label: 'Workspaces', | |
| path: '/', | |
| }, |
This can be removed - it's currently rendering an additional nav item "Workspaces" and duplicating the Workspaces table there. We are already rendering the Workspaces table from the "Notebooks" nav item.
…mockup kubeflow#203 Signed-off-by: Liav Weiss (EXT-Nokia) <liav.weiss.ext@nokia.com>
jenny-s51
left a comment
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
|
/lgtm |
|
/approve |
|
[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 |
|
@paulovmr, probably we will review all those UIs later, but I'm merging this as this will be incremental deliveries! |
…mockup kubeflow#203 (kubeflow#237) * feat(ws): Notebooks 2.0 // Frontend // Workspace Kind Viewer // Live mockup kubeflow#203 Signed-off-by: Liav Weiss (EXT-Nokia) <liav.weiss.ext@nokia.com> * feat(ws): Notebooks 2.0 // Frontend // Workspace Kind Viewer // Live mockup kubeflow#203 Signed-off-by: Liav Weiss (EXT-Nokia) <liav.weiss.ext@nokia.com> * feat(ws): Notebooks 2.0 // Frontend // Workspace Kind Viewer // Live mockup kubeflow#203 Signed-off-by: Liav Weiss (EXT-Nokia) <liav.weiss.ext@nokia.com> * feat(ws): Notebooks 2.0 // Frontend // Workspace Kind Viewer // Live mockup kubeflow#203 Signed-off-by: Liav Weiss (EXT-Nokia) <liav.weiss.ext@nokia.com> * feat(ws): Notebooks 2.0 // Frontend // Workspace Kind Viewer // Live mockup kubeflow#203 Signed-off-by: Liav Weiss (EXT-Nokia) <liav.weiss.ext@nokia.com> * feat(ws): Notebooks 2.0 // Frontend // Workspace Kind Viewer // Live mockup kubeflow#203 Signed-off-by: Liav Weiss (EXT-Nokia) <liav.weiss.ext@nokia.com> * feat(ws): Notebooks 2.0 // Frontend // Workspace Kind Viewer // Live mockup kubeflow#203 Signed-off-by: Liav Weiss (EXT-Nokia) <liav.weiss.ext@nokia.com> --------- Signed-off-by: Liav Weiss (EXT-Nokia) <liav.weiss.ext@nokia.com> Co-authored-by: Liav Weiss (EXT-Nokia) <liav.weiss.ext@nokia.com>
…mockup kubeflow#203 (kubeflow#237) * feat(ws): Notebooks 2.0 // Frontend // Workspace Kind Viewer // Live mockup kubeflow#203 Signed-off-by: Liav Weiss (EXT-Nokia) <liav.weiss.ext@nokia.com> * feat(ws): Notebooks 2.0 // Frontend // Workspace Kind Viewer // Live mockup kubeflow#203 Signed-off-by: Liav Weiss (EXT-Nokia) <liav.weiss.ext@nokia.com> * feat(ws): Notebooks 2.0 // Frontend // Workspace Kind Viewer // Live mockup kubeflow#203 Signed-off-by: Liav Weiss (EXT-Nokia) <liav.weiss.ext@nokia.com> * feat(ws): Notebooks 2.0 // Frontend // Workspace Kind Viewer // Live mockup kubeflow#203 Signed-off-by: Liav Weiss (EXT-Nokia) <liav.weiss.ext@nokia.com> * feat(ws): Notebooks 2.0 // Frontend // Workspace Kind Viewer // Live mockup kubeflow#203 Signed-off-by: Liav Weiss (EXT-Nokia) <liav.weiss.ext@nokia.com> * feat(ws): Notebooks 2.0 // Frontend // Workspace Kind Viewer // Live mockup kubeflow#203 Signed-off-by: Liav Weiss (EXT-Nokia) <liav.weiss.ext@nokia.com> * feat(ws): Notebooks 2.0 // Frontend // Workspace Kind Viewer // Live mockup kubeflow#203 Signed-off-by: Liav Weiss (EXT-Nokia) <liav.weiss.ext@nokia.com> --------- Signed-off-by: Liav Weiss (EXT-Nokia) <liav.weiss.ext@nokia.com> Co-authored-by: Liav Weiss (EXT-Nokia) <liav.weiss.ext@nokia.com>
…mockup kubeflow#203 (kubeflow#237) * feat(ws): Notebooks 2.0 // Frontend // Workspace Kind Viewer // Live mockup kubeflow#203 Signed-off-by: Liav Weiss (EXT-Nokia) <liav.weiss.ext@nokia.com> * feat(ws): Notebooks 2.0 // Frontend // Workspace Kind Viewer // Live mockup kubeflow#203 Signed-off-by: Liav Weiss (EXT-Nokia) <liav.weiss.ext@nokia.com> * feat(ws): Notebooks 2.0 // Frontend // Workspace Kind Viewer // Live mockup kubeflow#203 Signed-off-by: Liav Weiss (EXT-Nokia) <liav.weiss.ext@nokia.com> * feat(ws): Notebooks 2.0 // Frontend // Workspace Kind Viewer // Live mockup kubeflow#203 Signed-off-by: Liav Weiss (EXT-Nokia) <liav.weiss.ext@nokia.com> * feat(ws): Notebooks 2.0 // Frontend // Workspace Kind Viewer // Live mockup kubeflow#203 Signed-off-by: Liav Weiss (EXT-Nokia) <liav.weiss.ext@nokia.com> * feat(ws): Notebooks 2.0 // Frontend // Workspace Kind Viewer // Live mockup kubeflow#203 Signed-off-by: Liav Weiss (EXT-Nokia) <liav.weiss.ext@nokia.com> * feat(ws): Notebooks 2.0 // Frontend // Workspace Kind Viewer // Live mockup kubeflow#203 Signed-off-by: Liav Weiss (EXT-Nokia) <liav.weiss.ext@nokia.com> --------- Signed-off-by: Liav Weiss (EXT-Nokia) <liav.weiss.ext@nokia.com> Co-authored-by: Liav Weiss (EXT-Nokia) <liav.weiss.ext@nokia.com>
…mockup kubeflow#203 (kubeflow#237) * feat(ws): Notebooks 2.0 // Frontend // Workspace Kind Viewer // Live mockup kubeflow#203 Signed-off-by: Liav Weiss (EXT-Nokia) <liav.weiss.ext@nokia.com> * feat(ws): Notebooks 2.0 // Frontend // Workspace Kind Viewer // Live mockup kubeflow#203 Signed-off-by: Liav Weiss (EXT-Nokia) <liav.weiss.ext@nokia.com> * feat(ws): Notebooks 2.0 // Frontend // Workspace Kind Viewer // Live mockup kubeflow#203 Signed-off-by: Liav Weiss (EXT-Nokia) <liav.weiss.ext@nokia.com> * feat(ws): Notebooks 2.0 // Frontend // Workspace Kind Viewer // Live mockup kubeflow#203 Signed-off-by: Liav Weiss (EXT-Nokia) <liav.weiss.ext@nokia.com> * feat(ws): Notebooks 2.0 // Frontend // Workspace Kind Viewer // Live mockup kubeflow#203 Signed-off-by: Liav Weiss (EXT-Nokia) <liav.weiss.ext@nokia.com> * feat(ws): Notebooks 2.0 // Frontend // Workspace Kind Viewer // Live mockup kubeflow#203 Signed-off-by: Liav Weiss (EXT-Nokia) <liav.weiss.ext@nokia.com> * feat(ws): Notebooks 2.0 // Frontend // Workspace Kind Viewer // Live mockup kubeflow#203 Signed-off-by: Liav Weiss (EXT-Nokia) <liav.weiss.ext@nokia.com> --------- Signed-off-by: Liav Weiss (EXT-Nokia) <liav.weiss.ext@nokia.com> Co-authored-by: Liav Weiss (EXT-Nokia) <liav.weiss.ext@nokia.com> Signed-off-by: CI Bot <mkoushni@redhat.com>
…mockup kubeflow#203 (kubeflow#237) * feat(ws): Notebooks 2.0 // Frontend // Workspace Kind Viewer // Live mockup kubeflow#203 Signed-off-by: Liav Weiss (EXT-Nokia) <liav.weiss.ext@nokia.com> * feat(ws): Notebooks 2.0 // Frontend // Workspace Kind Viewer // Live mockup kubeflow#203 Signed-off-by: Liav Weiss (EXT-Nokia) <liav.weiss.ext@nokia.com> * feat(ws): Notebooks 2.0 // Frontend // Workspace Kind Viewer // Live mockup kubeflow#203 Signed-off-by: Liav Weiss (EXT-Nokia) <liav.weiss.ext@nokia.com> * feat(ws): Notebooks 2.0 // Frontend // Workspace Kind Viewer // Live mockup kubeflow#203 Signed-off-by: Liav Weiss (EXT-Nokia) <liav.weiss.ext@nokia.com> * feat(ws): Notebooks 2.0 // Frontend // Workspace Kind Viewer // Live mockup kubeflow#203 Signed-off-by: Liav Weiss (EXT-Nokia) <liav.weiss.ext@nokia.com> * feat(ws): Notebooks 2.0 // Frontend // Workspace Kind Viewer // Live mockup kubeflow#203 Signed-off-by: Liav Weiss (EXT-Nokia) <liav.weiss.ext@nokia.com> * feat(ws): Notebooks 2.0 // Frontend // Workspace Kind Viewer // Live mockup kubeflow#203 Signed-off-by: Liav Weiss (EXT-Nokia) <liav.weiss.ext@nokia.com> --------- Signed-off-by: Liav Weiss (EXT-Nokia) <liav.weiss.ext@nokia.com> Co-authored-by: Liav Weiss (EXT-Nokia) <liav.weiss.ext@nokia.com> Signed-off-by: CI Bot <mkoushni@redhat.com>
…mockup kubeflow#203 (kubeflow#237) * feat(ws): Notebooks 2.0 // Frontend // Workspace Kind Viewer // Live mockup kubeflow#203 Signed-off-by: Liav Weiss (EXT-Nokia) <liav.weiss.ext@nokia.com> * feat(ws): Notebooks 2.0 // Frontend // Workspace Kind Viewer // Live mockup kubeflow#203 Signed-off-by: Liav Weiss (EXT-Nokia) <liav.weiss.ext@nokia.com> * feat(ws): Notebooks 2.0 // Frontend // Workspace Kind Viewer // Live mockup kubeflow#203 Signed-off-by: Liav Weiss (EXT-Nokia) <liav.weiss.ext@nokia.com> * feat(ws): Notebooks 2.0 // Frontend // Workspace Kind Viewer // Live mockup kubeflow#203 Signed-off-by: Liav Weiss (EXT-Nokia) <liav.weiss.ext@nokia.com> * feat(ws): Notebooks 2.0 // Frontend // Workspace Kind Viewer // Live mockup kubeflow#203 Signed-off-by: Liav Weiss (EXT-Nokia) <liav.weiss.ext@nokia.com> * feat(ws): Notebooks 2.0 // Frontend // Workspace Kind Viewer // Live mockup kubeflow#203 Signed-off-by: Liav Weiss (EXT-Nokia) <liav.weiss.ext@nokia.com> * feat(ws): Notebooks 2.0 // Frontend // Workspace Kind Viewer // Live mockup kubeflow#203 Signed-off-by: Liav Weiss (EXT-Nokia) <liav.weiss.ext@nokia.com> --------- Signed-off-by: Liav Weiss (EXT-Nokia) <liav.weiss.ext@nokia.com> Co-authored-by: Liav Weiss (EXT-Nokia) <liav.weiss.ext@nokia.com>
Implementation for #203
workspaceKindsPage20.3.mp4
I also changed the deprecatedMessage to be shown only in case of Active status.