-
Notifications
You must be signed in to change notification settings - Fork 39
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
Pack Sidebar in its own module #463
Conversation
eca125e
to
69bc8d5
Compare
Deploying with
|
Latest commit: |
840b987
|
Status: | ✅ Deploy successful! |
Preview URL: | https://2ac9a670.console-overthinker-dev.pages.dev |
Branch Preview URL: | https://amnish04-sidebar-refactor.console-overthinker-dev.pages.dev |
4d3e39e
to
e6e01be
Compare
type SidebarMobileProps = SidebarContentProps & { | ||
searchText?: string; | ||
handleToggleSidebarVisible: () => void; | ||
isSidebarVisible: boolean; |
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.
SidebarMobile
and SidebarDesktop
both have isSidebarVisible
, could we put it in SidebarContent
?
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.
@WangGithub0 We probably can't have it in SidebarContent
as its not even using those props.
I was actually thinking of managing sidebar triggers inside the Sidebar
component which is a common ancestor of both SidebarDesktop
and SidebarMobile
.
But that came with a problem. Currently, many of the UI elements in the ChatBase
component are using isSidebarVisible
managed by the useDisclosure
hook at the top level of component hierarchy.
In other words, those elements are not reading directly from the Settings
context. And when I tried to encapsulate isSidebarVisible management in Sidebar.tsx
both of those properties got out of sync and started causing unexpected behaviours with opening and closing of sidebar.
Ideally, I think we should be managing the sidebarVisible
state with useSettings
hook considering that its state is meant to be single source of truth.
But since I have not used the useDisclosure
hook before and am not sure if its serving a higher purpose than my understanding, I decided not to touch it and pass that state as props to components. Again, I am also confused which path I should pick here, but the current way manages to make it work without making changes to lot of places
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.
Did some regression testing on your branch and things are working well
8fc3a25
to
840b987
Compare
#437 introduced a lot of duplication for sidebar related styles and logic as we now had different sidebar views for mobile and desktop.
I have tried to minimize that duplication, by creating separate components for mobile and desktop sidebar and exposing through a common
Sidebar
api, taking inspiration from the wayPromptForm
is done.This way, the places that use the sidebar now look the same as before since all the changes have been abstracted in its own module.
Here's a summary of changes:
Sidebar
component toSidebarContent
.SidebarMobile
component that usesSidebarContent
and extends it by adding the search field and ChatCraft brand at the top.SidebarDesktop
component wrapsSidebarContent
to apply the new open/close transition effect. This way, we don't have to repeat thekeyframe
definitions at evey place where sidebar is used.Sidebar
in index.tsx based on the screen width usingisMobile
hook.This fixes #457.