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

Added templating and access requests buttons to the new model create screen #1563

Conversation

ghost
Copy link

@ghost ghost commented Oct 14, 2024

image

@CLAassistant
Copy link

CLAassistant commented Oct 14, 2024

CLA assistant check
All committers have signed the CLA.

@@ -60,6 +62,8 @@ export default function CreateEntry({ createEntryKind, onBackClick }: CreateEntr
const [collaborators, setCollaborators] = useState<CollaboratorEntry[]>(
currentUser ? [{ entity: `${EntityKind.USER}:${currentUser?.dn}`, roles: ['owner'] }] : [],
)
const [ungovernedAccess, setungovernedAccess] = useState<boolean>(false)
Copy link
Member

Choose a reason for hiding this comment

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

boolean and string can be inferred from their defaults so we don't need to declare the types, so we can just do useState(false).

data-test='privateButtonSelector'
/>
</RadioGroup>
<Box sx={{ display: 'flex', flexDirection: 'column', gap: 1 }}>
Copy link
Member

Choose a reason for hiding this comment

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

We don't need to use this <Box/> component here

label={privateLabel()}
data-test='privateButtonSelector'
/>
<FormControlLabel
Copy link
Member

Choose a reason for hiding this comment

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

The option to allow for templating should appear at the bottom of the expansion menu under or above the choice to allow for ungoverned access. Although I can see why you'd put it under "Access control", this section is accessing the model card page - easy mistake to make though!

@@ -224,6 +256,16 @@ export default function CreateEntry({ createEntryKind, onBackClick }: CreateEntr
]}
/>
</Box>
<FormControlLabel
Copy link
Member

Choose a reason for hiding this comment

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

I would put the "allow templating" option here, and I'd suggest adding a sub-heading of "Additional settings", a bit like how the user selector appears under the heading of "Manage access list".

To easily space out different components you can use the MUI <Stack /> component, which works like so:

<Stack spacing={2}>
  <Typography>This is some text!</Typography>
  <Typography>This is some more text!</Typography>
</Stack>

@ghost ghost force-pushed the feature/BAI-1435-model-templating-ungoverned-access-and-templating-from-create-screen branch from 8ad64d8 to 6eed242 Compare October 14, 2024 15:27
@ghost
Copy link
Author

ghost commented Oct 14, 2024

image

@ghost ghost requested a review from ARADDCC002 October 14, 2024 15:54
@@ -130,6 +136,34 @@ export default function CreateEntry({ createEntryKind, onBackClick }: CreateEntr
)
}

const templateLabel = () => {
Copy link
Member

@ARADDCC002 ARADDCC002 Oct 14, 2024

Choose a reason for hiding this comment

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

const templateLabel = useMemo(() => {

)
}

const ungovAccessLabel = () => {
Copy link
Member

@ARADDCC002 ARADDCC002 Oct 14, 2024

Choose a reason for hiding this comment

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

const ungovAccessLabel = useMemo(() => {

I'd also consider renaming this to ungovernedAccessLabel as well

frontend/src/entry/CreateEntry.tsx Outdated Show resolved Hide resolved
@ghost ghost requested a review from ARADDCC002 October 15, 2024 12:12
@ghost
Copy link
Author

ghost commented Oct 15, 2024

image

no UI changes

@ghost ghost requested a review from ARADDCC012 October 15, 2024 13:26
@@ -130,6 +135,42 @@ export default function CreateEntry({ createEntryKind, onBackClick }: CreateEntr
)
}

const TemplateLabel = ({ createEntryKind }) => {
Copy link
Member

Choose a reason for hiding this comment

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

This should just be (I suggest changing the name to be clear that it in itself isn't a template):

const allowTemplatingLabel = useMemo(() => {
      return (
        <Stack direction='row' justifyContent='center' alignItems='center' spacing={1}>
          <FolderCopy />
          <Stack sx={{ my: 1 }}>
            <Typography fontWeight='bold'>Templating</Typography>
            <Typography variant='caption'>
              {`Allow this to be used as a template for another ${EntryKindLabel[createEntryKind]}`}
            </Typography>
          </Stack>
        </Stack>
      )
    }, [createEntryKind])

It's important to note that hooks in react have to be called in the same order on every render cycle, so they should always be defined at the base of the component.

I suggest having a quick read on how memo hooks work in particular, but in short this is for performance reasons. https://react.dev/reference/react/useMemo

return CreateTemplateLabel
}

const UngovernedAccessLabel = ({ createEntryKind }) => {
Copy link
Member

Choose a reason for hiding this comment

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

Same here, this should be:

    const ungovernedAccessLabel = useMemo(() => {
      return (
        <Stack direction='row' justifyContent='center' alignItems='center' spacing={1}>
          <Gavel />
          <Stack sx={{ my: 1 }}>
            <Typography fontWeight='bold'>Ungoverned Access Requests</Typography>
            <Typography variant='caption'>
              {`Allow users to request access without the need for authorisation from ${EntryKindLabel[createEntryKind]} owners`}
            </Typography>
          </Stack>
        </Stack>
      )
    }, [createEntryKind])

size='small'
/>
}
label={UngovernedAccessLabel({ createEntryKind })}
Copy link
Member

Choose a reason for hiding this comment

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

This should be label={ungovernedAccessLabel}

size='small'
/>
}
label={TemplateLabel({ createEntryKind })}
Copy link
Member

Choose a reason for hiding this comment

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

This should be label={allowTemplatingLabel}

@ghost ghost merged commit e9b8b97 into main Oct 16, 2024
15 checks passed
@ghost ghost deleted the feature/BAI-1435-model-templating-ungoverned-access-and-templating-from-create-screen branch October 16, 2024 09:23
@ghost ghost added the enhancement New feature or request label Oct 22, 2024
@ghost ghost self-assigned this Oct 22, 2024
This pull request was closed.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants