-
Notifications
You must be signed in to change notification settings - Fork 15
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: add a top-right share-button on the repo page #152
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.
Thanks, Xinyi! This feature runs very well. Overall the code looks great to me. I left a couple of comments.
<MenuItem onClick={props.onShareClick} sx={ItemStyle}> | ||
<ListItemIcon> | ||
<ShareOutlinedIcon /> | ||
</ListItemIcon> | ||
<ListItemText> Share </ListItemText> | ||
</MenuItem> |
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 could remove this right-click menu item now.
ui/src/components/Header.tsx
Outdated
inRepo = false, | ||
setShareOpen = () => {}, |
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.
- It seems that
setShareOpen
doesn't need to be passed in. You could calluseStore
to get it. inRepo
isn't general enough. I would use abuttonItem=null
argument and pass in a ReactNode (just like thebreadcrumbItem
above). This way, the repo sharing UI & logic are not leaked intoHeader
's implementation.
E.g.,
// repo.tsx
function ShareButton() {
const setShareOpen = useStore(...);
return <Box><Button onClick={()=>setShareOpen(true)}>...</Button></Box>
}
function RepoWrapper() {
return <Box> ... <Header buttonItem={<ShareButton/>}/></Box>
}
// Header.tsx
function Header({..., buttonItem=null}) {
return <Box> ... {buttonItem} ... </Box>
}
Update:
|
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 is a great feature and code, thanks!
Change:
isPublic
and the list of collaborators) loads immediately after actions.By the way, if there are no collaborators now, it looks like:
Please test carefully before merging.
Related to-do list:
useReducer
if I have time.