-
Notifications
You must be signed in to change notification settings - Fork 26
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
Sidebar Enhancement Reboot #437
Conversation
Deploying with
|
Latest commit: |
d008d45
|
Status: | ✅ Deploy successful! |
Preview URL: | https://36b25199.console-overthinker-dev.pages.dev |
Branch Preview URL: | https://amnish04-issue-299.console-overthinker-dev.pages.dev |
71edb92
to
e4d1737
Compare
@Amnish04 can you rebase this, and add the placeholder change there too? |
67448fd
to
d008d45
Compare
@humphd Sure, just rebased |
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 fantastic improvement. The only thing I wonder about, as I read this, is the amount of code duplication. However, we have that now, too.
const isMobile = useMobileBreakpoint(); | ||
const sidebarColor = useColorModeValue("blue.600", "blue.200"); | ||
|
||
const sidebarOpenAnimationKeyframes = keyframes` |
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're repeating a bunch of code. It would be good to think about how we might reuse any of this down the road (doesn't need to block this work).
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 agree, this duplication has been itching me as well. To address this, I have packed the entire sidebar into its own module like we are doing the PromptFrom
.
But I don't want to push it on this branch as it might delay this PR. Can I merge this now, so I can open another PR right away as a follow up for this?
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.
@Amnish04 I tested it and the mobile changes are really good - big improvement from what it is like in production right now (in prod right now if you have the mobile and the sidebar open and refresh the app becomes unuseable)
Great, thanks for reviewing! |
Go ahead and land it, then improve in follow-ups. |
Pull Request #300 has been open for a while, and since then a lot of our preferences have changed regarding sidebar.
I really wanted to get it done, but would have been really difficult to continue on that branch.
Which is why, I have redone the sidebar enhancement from scratch.
Here's how it looks:
On Desktop:
![SidebarRebootDesktop](https://private-user-images.githubusercontent.com/78865303/303812283-dfaa70e8-dbbe-4ce1-8035-e77b75937b85.gif?jwt=eyJhbGciOiJIUzI1NiIsInR5cCI6IkpXVCJ9.eyJpc3MiOiJnaXRodWIuY29tIiwiYXVkIjoicmF3LmdpdGh1YnVzZXJjb250ZW50LmNvbSIsImtleSI6ImtleTUiLCJleHAiOjE3MjAwMTY3NzUsIm5iZiI6MTcyMDAxNjQ3NSwicGF0aCI6Ii83ODg2NTMwMy8zMDM4MTIyODMtZGZhYTcwZTgtZGJiZS00Y2UxLTgwMzUtZTc3Yjc1OTM3Yjg1LmdpZj9YLUFtei1BbGdvcml0aG09QVdTNC1ITUFDLVNIQTI1NiZYLUFtei1DcmVkZW50aWFsPUFLSUFWQ09EWUxTQTUzUFFLNFpBJTJGMjAyNDA3MDMlMkZ1cy1lYXN0LTElMkZzMyUyRmF3czRfcmVxdWVzdCZYLUFtei1EYXRlPTIwMjQwNzAzVDE0MjExNVomWC1BbXotRXhwaXJlcz0zMDAmWC1BbXotU2lnbmF0dXJlPWUwODkyZjQ5MWQ3YmYxNDM2ZjgwMjQ5MjExMmQ1NmIxMzRlOGIxYjIwNGY3NDRhYzFlMWUwMDc2MjZhZThjZDEmWC1BbXotU2lnbmVkSGVhZGVycz1ob3N0JmFjdG9yX2lkPTAma2V5X2lkPTAmcmVwb19pZD0wIn0.BEbVQC5J9rtlmbAJz-zKgv_vnjQ1TTV16uCMHQIDyXg)
On Mobile:
![SidebarRebootMobile](https://private-user-images.githubusercontent.com/78865303/303812604-f64707d2-1c1e-459f-bdaf-c7430aabda17.gif?jwt=eyJhbGciOiJIUzI1NiIsInR5cCI6IkpXVCJ9.eyJpc3MiOiJnaXRodWIuY29tIiwiYXVkIjoicmF3LmdpdGh1YnVzZXJjb250ZW50LmNvbSIsImtleSI6ImtleTUiLCJleHAiOjE3MjAwMTY3NzUsIm5iZiI6MTcyMDAxNjQ3NSwicGF0aCI6Ii83ODg2NTMwMy8zMDM4MTI2MDQtZjY0NzA3ZDItMWMxZS00NTlmLWJkYWYtYzc0MzBhYWJkYTE3LmdpZj9YLUFtei1BbGdvcml0aG09QVdTNC1ITUFDLVNIQTI1NiZYLUFtei1DcmVkZW50aWFsPUFLSUFWQ09EWUxTQTUzUFFLNFpBJTJGMjAyNDA3MDMlMkZ1cy1lYXN0LTElMkZzMyUyRmF3czRfcmVxdWVzdCZYLUFtei1EYXRlPTIwMjQwNzAzVDE0MjExNVomWC1BbXotRXhwaXJlcz0zMDAmWC1BbXotU2lnbmF0dXJlPTM2M2QwYzc1Mzk2YjJiNmNkOTdmZmUwMzIzYWM1ZGVkZDZlMTMwOTdiNDg5ZTI3YmRmNzlkN2Y1YzU2NmI2ZTImWC1BbXotU2lnbmVkSGVhZGVycz1ob3N0JmFjdG9yX2lkPTAma2V5X2lkPTAmcmVwb19pZD0wIn0.C2IIW8kPboJMDeqO4Wauv5MHfAFI9oNg8KOpPw3bjlQ)
Breakpoint:
![SidebarRebootBreakpoint](https://private-user-images.githubusercontent.com/78865303/303812713-2dfa6aa9-eeeb-46df-b965-4a1725144900.gif?jwt=eyJhbGciOiJIUzI1NiIsInR5cCI6IkpXVCJ9.eyJpc3MiOiJnaXRodWIuY29tIiwiYXVkIjoicmF3LmdpdGh1YnVzZXJjb250ZW50LmNvbSIsImtleSI6ImtleTUiLCJleHAiOjE3MjAwMTY3NzUsIm5iZiI6MTcyMDAxNjQ3NSwicGF0aCI6Ii83ODg2NTMwMy8zMDM4MTI3MTMtMmRmYTZhYTktZWVlYi00NmRmLWI5NjUtNGExNzI1MTQ0OTAwLmdpZj9YLUFtei1BbGdvcml0aG09QVdTNC1ITUFDLVNIQTI1NiZYLUFtei1DcmVkZW50aWFsPUFLSUFWQ09EWUxTQTUzUFFLNFpBJTJGMjAyNDA3MDMlMkZ1cy1lYXN0LTElMkZzMyUyRmF3czRfcmVxdWVzdCZYLUFtei1EYXRlPTIwMjQwNzAzVDE0MjExNVomWC1BbXotRXhwaXJlcz0zMDAmWC1BbXotU2lnbmF0dXJlPTBhOWZhMWUxM2VlMTYzMjA4NTgyYTZiNDA3NDg1ZTkwODFlM2Y0NGYwMzliM2ZhOWM1ZjIzOGZjZTExOGM3MDMmWC1BbXotU2lnbmVkSGVhZGVycz1ob3N0JmFjdG9yX2lkPTAma2V5X2lkPTAmcmVwb19pZD0wIn0.OWjV9HYJ7F4rrn9TVHsfbdN3oCPYID-vrcu5yzZJWDY)
This fixes #299