Skip to content
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: ✨ responsive sidebar #1279

Merged
merged 16 commits into from
Oct 2, 2023
Merged

feat: ✨ responsive sidebar #1279

merged 16 commits into from
Oct 2, 2023

Conversation

matthieujacq
Copy link
Contributor

Description

  • ✨ responsive sidebar
  • 💄 new design for the buttons in the sidebar
  • 🧩 More modular architecture
  • 🐛 Fix the sidebar quick opening and close on mobile

Screenshots (if appropriate):

@matthieujacq matthieujacq temporarily deployed to preview September 28, 2023 10:12 — with GitHub Actions Inactive
@dosubot dosubot bot added the area: frontend Related to frontend functionality or under the /frontend directory label Sep 28, 2023
@vercel
Copy link

vercel bot commented Sep 28, 2023

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Comments Updated (UTC)
docs ✅ Ready (Inspect) Visit Preview 💬 Add feedback Oct 2, 2023 0:06am
quivr-strapi ✅ Ready (Inspect) Visit Preview 💬 Add feedback Oct 2, 2023 0:06am
quivrapp ✅ Ready (Inspect) Visit Preview 💬 Add feedback Oct 2, 2023 0:06am

@github-actions
Copy link
Contributor

github-actions bot commented Sep 28, 2023

Risk Level 2 - /home/runner/work/quivr/quivr/frontend/app/chat/components/ChatsList/components/ChatHistory.tsx

The code seems to be well written and follows the SOLID principles. However, there is a potential performance issue. The filter functions are called directly inside the component. This means that the filtering will be executed on every render of the component, even if the allChats array has not changed. To improve performance, consider using the useMemo hook to only recompute the filtered arrays when allChats changes.

const todayChats = useMemo(() => allChats.filter((chat) =>
  isToday(new Date(chat.creation_time))
), [allChats]);

Do the same for yesterdayChats, last7DaysChats, and last30DaysChats.


Risk Level 2 - /home/runner/work/quivr/quivr/frontend/lib/components/Sidebar/Sidebar.tsx

The code seems to be well written and follows the SOLID principles. However, there is a potential performance issue. The motion.div component is used with the drag prop set to x. This means that the component will be draggable along the x-axis. However, the dragConstraints prop is also set, which limits the dragging to the right and left. This could potentially cause performance issues if the component is large or complex. Consider removing the drag prop if it is not necessary.


Risk Level 2 - /home/runner/work/quivr/quivr/frontend/app/brains-management/[brainId]/components/BrainsList/BrainsList.tsx

The code seems to be well written and follows the SOLID principles. However, there is a potential performance issue. The brains.map function is called directly inside the JSX code. This means that the mapping will be executed on every render of the component, even if the brains array has not changed. To improve performance, consider using the useMemo hook to only recompute the mapped array when brains changes.

const renderedBrains = useMemo(() => brains.map((brain) => (
  <BrainListItem brain={brain} key={brain.id} />
)), [brains]);

Then use renderedBrains in your JSX code.


📝🚀🔧


Powered by Code Review GPT

Comment on lines +15 to +17
{path === null ||
path.startsWith("/chat") ||
path.startsWith("/brains-management") ? (
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For improved readability, you can:

  1. Declare a constant myConst equal to your checks.
  2. Implement an early return, so if myConst is not true, return ...

@matthieujacq matthieujacq merged commit c140b9c into main Oct 2, 2023
11 checks passed
@mamadoudicko
Copy link
Contributor

Nice PR @matthieujacq!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area: frontend Related to frontend functionality or under the /frontend directory
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants