Skip to content
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-846 update workspace pages #3537

Merged
merged 29 commits into from
Sep 13, 2024

Conversation

austenem
Copy link
Collaborator

Summary

This update adds a workspaces templates landing page, which is linked in the workspaces landing page, as well as template detail pages. These detail pages include information about the templates as well as example workspaces, which can be cloned from these pages via a new "Try Sample Workspace" dialog.

Design Documentation/Original Tickets

CAT-846 Jira ticket

CAT-819 Jira ticket (design)

Figma mockups

Testing

Template examples were tested on DEV using the one template that currently has examples (squidpy), and other examples were pulled from the most recent user-templates-api PR and hardcoded into the component for testing.

Note: attempting to clone the squidpy example workspace on DEV was unsuccessful due to the attached dataset only being available on PROD. Cloning on PROD with a hardcoded version of the example workspace was successful.

Screenshots/Video

Updated workspaces landing page:

Screenshot 2024-09-11 at 11 15 35 AM

New templates landing page:

Screenshot 2024-09-11 at 11 14 30 AM

New template detail page:

Logged in (with example):

Screenshot 2024-09-11 at 11 13 12 AM

Logged out (with example):

Screenshot 2024-09-11 at 11 22 07 AM

Logged in (no example):

Screenshot 2024-09-11 at 11 21 13 AM

New "Try Sample Workspace" dialog:

Screenshot 2024-09-11 at 11 14 05 AM

Checklist

  • Code follows the project's coding standards
    • Lint checks pass locally
    • New CHANGELOG-your-feature-name-here.md is present in the root directory, describing the change(s) in full sentences.
  • Unit tests covering the new feature have been added
  • All existing tests pass
  • Any relevant documentation in JIRA/Confluence has been updated to reflect the new feature
  • Any new functionalities have appropriate analytics functionalities added -> (future ticket)

@john-conroy
Copy link
Collaborator

Looks great! I'm working through the changes, but should we set the initial sample workspace name to the title of the example?

@austenem
Copy link
Collaborator Author

I think that's a good idea!

@austenem austenem changed the title Austenem/cat 846 update workspace pages Austenem/CAT-846 update workspace pages Sep 11, 2024
Copy link
Collaborator

@john-conroy john-conroy left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A few questions for now. I still have to check it out locally.

const { isOpen: isLaunchWorkspaceDialogOpen } = useLaunchWorkspaceStore();

const { data } = useJobTypes();
const jobTypeKey = example.job_type ?? DEFAULT_JOB_TYPE;
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this a placeholder? We don't have job_type in the examples yet. We should set this to DEFAULT_JOB_TYPE and leave a TODO comment.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Personally, I'm okay with this as long as we know the field will be called job_type - it might save us an extra deploy in the long run

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I added optional job_type and resource_options values to the TemplateExample type, although neither have been added to the examples yet - both default to the default values if they are not included.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@austenem this will be a job_types array.

Comment on lines -55 to -58
<Typography sx={{ mt: 2 }} variant="subtitle1">
Filter workspace templates by tags
</Typography>
<Stack spacing={1}>
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The TemplateSelectStep is used across a number of forms. The designs no longer include this for any of them?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is a stylistic change to update the tag search to match designs:

Current version:
Screenshot 2024-09-11 at 1 30 01 PM

Updated:
Screenshot 2024-09-11 at 1 30 13 PM

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah, it does actually look like the current designs for the launch workspace dialogs still include this title - good catch!

context/app/static/js/pages/Template/Template.tsx Outdated Show resolved Hide resolved
context/app/static/js/pages/Template/Template.tsx Outdated Show resolved Hide resolved
const FlexContainer = styled('div')(({ theme }) => ({
display: 'flex',
alignItems: 'center',
marginBottom: theme.spacing(2),
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Don't we need the marginBottom for all of the other instances of IconPageTitle?

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I couldn't find any references to IconPageTitle outside of workspaces, so this might be a duplicate case:
image

In the other cases, the SummaryData component appears to be handling the display of this title and icon:

<SummaryTitle data-testid="entity-type">
<SummaryDataHeader>
<StyledSvgIcon as={entityIconMap[entity_type]} color="primary" />
{entityTypeDisplay ?? entity_type}
</SummaryDataHeader>

context/app/static/js/shared-styles/surfaces/Step/Step.tsx Outdated Show resolved Hide resolved
@@ -34,7 +35,7 @@ function StepDescription({ blocks }: { blocks: (string | ReactElement)[] }) {
</Stack>
);
}
function Step({ index, title, isRequired = false, children }: PropsWithChildren<StepProps>) {
function Step({ index, title, isRequired = false, hideRequiredText, children }: PropsWithChildren<StepProps>) {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do we need to set a default for hideRequiredText to false for the other, existing uses of Step or does this reflect the designs?

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If there's no default value set, it's falsy by default due to being undefined, so it should fall back to the existing behavior.

Copy link
Collaborator

@NickAkhmetov NickAkhmetov left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just a few places I noticed some potential duplications between our components, no major concerns 🙂

context/app/static/js/pages/Template/Template.tsx Outdated Show resolved Hide resolved
import AccordionSummary from '@mui/material/AccordionSummary';
import { styled } from '@mui/material/styles';

const StyledAccordionSummary = styled(AccordionSummary)(({ theme }) => {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

These styles seem pretty similar to the ones for the Processed Dataset accordion: https://github.com/hubmapconsortium/portal-ui/blob/25b9fea3fca9f6bc8d789b6c8851f7e44779e213/context/app/static/js/components/detailPage/ProcessedData/ProcessedDataset/styles.ts

Maybe there's some deduplication we could do? It doesn't have to be in this PR, but we should file a ticket otherwise.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh nice! Good call.

context/app/static/js/pages/Templates/Templates.tsx Outdated Show resolved Hide resolved
const FlexContainer = styled('div')(({ theme }) => ({
display: 'flex',
alignItems: 'center',
marginBottom: theme.spacing(2),
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I couldn't find any references to IconPageTitle outside of workspaces, so this might be a duplicate case:
image

In the other cases, the SummaryData component appears to be handling the display of this title and icon:

<SummaryTitle data-testid="entity-type">
<SummaryDataHeader>
<StyledSvgIcon as={entityIconMap[entity_type]} color="primary" />
{entityTypeDisplay ?? entity_type}
</SummaryDataHeader>

@@ -34,7 +35,7 @@ function StepDescription({ blocks }: { blocks: (string | ReactElement)[] }) {
</Stack>
);
}
function Step({ index, title, isRequired = false, children }: PropsWithChildren<StepProps>) {
function Step({ index, title, isRequired = false, hideRequiredText, children }: PropsWithChildren<StepProps>) {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If there's no default value set, it's falsy by default due to being undefined, so it should fall back to the existing behavior.

john-conroy
john-conroy previously approved these changes Sep 12, 2024
Copy link
Collaborator

@john-conroy john-conroy left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good to me. Let's hold off on merging this until we have the examples on PROD.

@john-conroy
Copy link
Collaborator

Sorry to comment after already approving. Can we make sure the sort order for the templates grid is consistent across pages/forms?

@austenem
Copy link
Collaborator Author

Sorry to comment after already approving. Can we make sure the sort order for the templates grid is consistent across pages/forms?

Ah good catch! All template grids should be sorted now, not just the selectable ones.

@john-conroy
Copy link
Collaborator

Good to merge!

@austenem austenem merged commit 47a6036 into main Sep 13, 2024
8 checks passed
@austenem austenem deleted the austenem/cat-846-update-workspace-pages branch September 13, 2024 16:35
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants