-
Notifications
You must be signed in to change notification settings - Fork 0
Project Page Detail Part 4 #50
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
base: main
Are you sure you want to change the base?
Conversation
…il-page-adding-the-page
…-detail-page-adding-the-detail-page
…-detail-page-adding-the-detail-page
…-detail-page-adding-the-detail-page
| breadcrumbs={[ | ||
| {title: "Home", href: `/course/${courseId}`}, | ||
| {title: "Project Sets", href: `/course/${courseId}/project-sets`}, | ||
| {title: `Project Set Detail`, href: `/course/${courseId}/project-sets/${projectSetId}`}, |
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.
"Project Set Detail" provides no value as a breadcrumb item.
At minimum, it should just be the name of the project set
| } | ||
|
|
||
| export const ProjectsProvider: FC<PropsWithChildren> = ({children}) => { | ||
| const projectSet = useProjects() |
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.
nit;
do we really gain value fro making an entire other hook just so we can use it in one place and set all the values in the provider. Seems like we could just do all the useState and useEffect stuff without putting it in another hook
| const ProjectSetContext = createContext<ProjectSetContextType>({ | ||
| projectSetId: "", | ||
| projects: [], | ||
| displayProjects: [], |
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.
still reviewing but have my doubts if we need the logic of displayProjects to exist at the provider level. I kinda think the left sidebar could handle this logic without anything else under the context needing to be aware of it
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.
let's not do this imo
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.
idk why this is a context tbh? This seems like straight internal behaviour of the search sidebar on its own and really didn't need to be a context. Nothing else consumes this and I don't think anything ever will
| setDisplayProjects(filteredProjects) | ||
| setSearchText(searchText) | ||
|
|
||
| // If the current project is not in the filtered projects, set the current project to the first filtered project |
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.
idea;
I think a nice UX flow would be to have the right side of the page render a nice empty state whenever the search changes. Then make it possible to set null for the current project. Then whenever the search changes we set the current project to null.
This PR includes:
Demo:
Screen.Recording.2024-05-13.at.8.28.41.PM.mov