-
Notifications
You must be signed in to change notification settings - Fork 2
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
Austenem/CAT-1150 Workspaces landing page #3700
base: austenem/workspace-sharing
Are you sure you want to change the base?
Austenem/CAT-1150 Workspaces landing page #3700
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.
Nice progress! I understand everything is in flux so no need to address these immediately.
* @param field The field to extract the prefix from. | ||
* @returns 'original_workspace_id.' or 'shared_workspace_id.' if the field is a workspace invitation, else an empty string. | ||
*/ | ||
function getFieldPrefix(field: string) { |
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.
Is this used anywhere?
border: 'none', | ||
sx: { border: 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.
What does the sx
here achieve?
return launchDate || createdDate; | ||
} | ||
|
||
const fieldWithPrefix = prefix ? `${prefix}${field}` : field; |
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.
If we add a default of ''
for the prefix we can avoid the ternary.
function getFieldValue({ item, field, prefix }: { item: WorkspaceItem; field: string; prefix?: string }) { | ||
// datetime_last_job_launch starts as null, so we fall back to datetime_created if the workspace has never been launched | ||
if (field === 'datetime_last_job_launch') { | ||
const launchDate = get(item, field, ''); |
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.
Do you prefer the get
syntax rather than item?.[field] ?? ''
?
const workspacesWithInvitationInfo = workspacesList.map((workspace) => { | ||
const user_id = receivedInvitations.find((invitation) => invitation.original_workspace_id.id === workspace.id) | ||
?.shared_workspace_id.user_id; | ||
|
||
return { | ||
...workspace, | ||
user_id, | ||
}; | ||
}); |
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.
Could it make sense to first make a map of the invitations by ID to avoid the nested looping? Is there a situation in which you have multiple invitations with the same original_workspace_id
?
const [inputValue, setInputValue] = useState(''); | ||
|
||
// Filter workspaces in list based on search input | ||
const filteredWorkspaces = useMemo(() => { | ||
if (!inputValue.trim()) return workspacesList; | ||
return workspacesList.filter( | ||
(ws) => | ||
ws.name.toLowerCase().includes(inputValue.toLowerCase()) || ws.id.toString().includes(inputValue.toLowerCase()), | ||
); | ||
}, [inputValue, workspacesList]); |
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.
Should we introduce some debounce
to avoid filtering the workspaces on every change to the input?
|
||
useEffect(() => { | ||
setShowShared(sharedCount > 0); | ||
}, [sharedCount]); |
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 just pass the sharedCount > 0
as the default value to the useState
for showShared
?
const { ownCount, sharedCount } = useMemo(() => { | ||
const own = workspacesList.filter((workspace) => !workspace.user_id).length; | ||
return { ownCount: own, sharedCount: workspacesList.length - own }; | ||
}, [workspacesList]); |
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.
Instead of returning the lengths here could we filter/split the workspaceList by own and shared here? Then we wouldn't have to filter each time the showOwn and showShared state change.
items.forEach((item) => { | ||
const itemId = 'id' in item ? item.id.toString() : item.original_workspace_id.id.toString(); | ||
if (selectedItemIds.size === items.length) { | ||
toggleItem(itemId); | ||
} else if (!selectedItemIds.has(itemId)) { | ||
toggleItem(itemId); | ||
} |
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 add a toggleAllItems
or similar and avoid toggling each item individually?
const pending = invitations.filter((inv) => !inv.is_accepted).length; | ||
return { pendingCount: pending, acceptedCount: invitations.length - pending }; | ||
}, [invitations]); | ||
|
||
const [showPending, setShowPending] = useState(false); | ||
const [showAccepted, setShowAccepted] = useState(false); | ||
|
||
useEffect(() => { | ||
setShowPending(pendingCount > 0); | ||
}, [pendingCount]); | ||
|
||
const filteredInvitations = useMemo( | ||
() => | ||
[...invitations].filter((invitation) => { | ||
if (showPending && !invitation.is_accepted) return true; | ||
if (showAccepted && invitation.is_accepted) return true; | ||
return false; | ||
}), | ||
[invitations, showPending, showAccepted], | ||
); |
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.
Same comment as the one in workspaces/Tables/WorkspacesTable/hooks.ts
.
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.
Looks good! Agreed with John's comments + a minor request on my part.
WorkspaceButton.defaultProps = { | ||
size: 'large', | ||
size: 'small', | ||
}; |
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.
defaultProps
is deprecated ahead of React 19 - could we provide this as part of the styled component instead? e.g. something like:
const WorkspaceButton = styled((props: React.ComponentProps<TooltipIconButton> => <TooltipIconButton size="large" {...props} />)(({ theme }) => ({
backgroundColor: theme.palette.white.main,
color: theme.palette.primary.main,
borderRadius: theme.spacing(0.5),
border: `1px solid ${theme.palette.divider}`,
}));
Summary
Updates workspaces landing page with tables for workspace invitations and owned workspaces. First PR for in-progress workspace sharing implementation.
Design Documentation/Original Tickets
CAT-1150 Jira ticket
Testing
Tested via interacting with endpoints directly to share workspaces back and forth, as sharing dialogs are not yet implemented.
Screenshots/Video
Checklist
CHANGELOG-your-feature-name-here.md
is present in the root directory, describing the change(s) in full sentences.