-
Notifications
You must be signed in to change notification settings - Fork 13
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
Added templating and access requests buttons to the new model create screen #1563
Conversation
ghost
commented
Oct 14, 2024
frontend/src/entry/CreateEntry.tsx
Outdated
@@ -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) |
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.
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)
.
frontend/src/entry/CreateEntry.tsx
Outdated
data-test='privateButtonSelector' | ||
/> | ||
</RadioGroup> | ||
<Box sx={{ display: 'flex', flexDirection: 'column', gap: 1 }}> |
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.
We don't need to use this <Box/>
component here
frontend/src/entry/CreateEntry.tsx
Outdated
label={privateLabel()} | ||
data-test='privateButtonSelector' | ||
/> | ||
<FormControlLabel |
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 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!
frontend/src/entry/CreateEntry.tsx
Outdated
@@ -224,6 +256,16 @@ export default function CreateEntry({ createEntryKind, onBackClick }: CreateEntr | |||
]} | |||
/> | |||
</Box> | |||
<FormControlLabel |
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.
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>
…-access-and-templating-from-create-screen
8ad64d8
to
6eed242
Compare
frontend/src/entry/CreateEntry.tsx
Outdated
@@ -130,6 +136,34 @@ export default function CreateEntry({ createEntryKind, onBackClick }: CreateEntr | |||
) | |||
} | |||
|
|||
const templateLabel = () => { |
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 templateLabel = useMemo(() => {
frontend/src/entry/CreateEntry.tsx
Outdated
) | ||
} | ||
|
||
const ungovAccessLabel = () => { |
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 ungovAccessLabel = useMemo(() => {
I'd also consider renaming this to ungovernedAccessLabel
as well
…outing and memoization
frontend/src/entry/CreateEntry.tsx
Outdated
@@ -130,6 +135,42 @@ export default function CreateEntry({ createEntryKind, onBackClick }: CreateEntr | |||
) | |||
} | |||
|
|||
const TemplateLabel = ({ createEntryKind }) => { |
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.
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
frontend/src/entry/CreateEntry.tsx
Outdated
return CreateTemplateLabel | ||
} | ||
|
||
const UngovernedAccessLabel = ({ createEntryKind }) => { |
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 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])
frontend/src/entry/CreateEntry.tsx
Outdated
size='small' | ||
/> | ||
} | ||
label={UngovernedAccessLabel({ createEntryKind })} |
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.
This should be label={ungovernedAccessLabel}
frontend/src/entry/CreateEntry.tsx
Outdated
size='small' | ||
/> | ||
} | ||
label={TemplateLabel({ createEntryKind })} |
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.
This should be label={allowTemplatingLabel}