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

style: improve ui #1263

Merged
merged 10 commits into from
Sep 27, 2023
Merged

style: improve ui #1263

merged 10 commits into from
Sep 27, 2023

Conversation

mamadoudicko
Copy link
Contributor

@mamadoudicko mamadoudicko commented Sep 26, 2023

  • align brain acces status change confirmation buttons
  • reduce remove/unsubscribe button margin
  • align confirmationModal buttons
  • update modal display status logic
  • add brains management button
  • improve public brain item ui
  • style: make brain library responsive
Screen.Recording.2023-09-26.at.12.05.28.mp4

@vercel
Copy link

vercel bot commented Sep 26, 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 Sep 27, 2023 7:43am
quivr-strapi ✅ Ready (Inspect) Visit Preview 💬 Add feedback Sep 27, 2023 7:43am
quivrapp ✅ Ready (Inspect) Visit Preview 💬 Add feedback Sep 27, 2023 7:43am

@dosubot dosubot bot added the area: frontend Related to frontend functionality or under the /frontend directory label Sep 26, 2023
@mamadoudicko mamadoudicko temporarily deployed to preview September 26, 2023 10:03 — with GitHub Actions Inactive
@github-actions
Copy link
Contributor

github-actions bot commented Sep 26, 2023

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

The code seems to be well written and follows the SOLID principles. However, there are a few areas that could be improved for better readability and maintainability:

  1. Use of Ternary Operator: The ternary operator is used multiple times in the code. While it's not a problem, it can make the code harder to read when overused. Consider using if-else statements for better readability.

  2. Type Definitions: The ModalProps type is defined as a union type which can be a bit confusing. Consider simplifying the type definitions if possible.

  3. Use of useState: The useState hook is used to manage the isOpen state. However, it seems like the customIsOpen and customSetOpen props can override this state. This can lead to confusion and potential bugs in the future. Consider revising the state management in this component.

Here's an example of how you might simplify the type definitions:

type ModalProps = {
  title?: string;
  desc?: string;
  children?: ReactNode;
  Trigger?: ReactNode;
  CloseTrigger?: ReactNode;
  isOpen: boolean;
  setOpen: (isOpen: boolean) => void;
};

And here's an example of how you might revise the state management:

const isOpen = customIsOpen ?? useState(false);
const setOpen = customSetOpen ?? useState(false);

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 good practices. However, there is a potential risk of the brains being undefined. This is handled by returning an empty div but it might be better to handle this scenario more gracefully. For example, you could display a message to the user or redirect them to a different page.

if (brains === undefined) {
  return <div>No brains available</div>;
}

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

The code seems to be well written and follows good practices. However, there is a potential risk of the brainId being undefined. This is handled by returning an empty div but it might be better to handle this scenario more gracefully. For example, you could display a message to the user or redirect them to a different page.

if (brainId === undefined) {
  return <div>No brain selected</div>;
}

📖🔍🔧


Powered by Code Review GPT

@mamadoudicko mamadoudicko temporarily deployed to preview September 26, 2023 10:18 — with GitHub Actions Inactive
@mamadoudicko mamadoudicko temporarily deployed to preview September 26, 2023 10:19 — with GitHub Actions Inactive
@mamadoudicko mamadoudicko temporarily deployed to preview September 26, 2023 13:42 — with GitHub Actions Inactive
@mamadoudicko mamadoudicko temporarily deployed to preview September 26, 2023 13:43 — with GitHub Actions Inactive
@mamadoudicko mamadoudicko temporarily deployed to preview September 26, 2023 16:45 — with GitHub Actions Inactive
gozineb
gozineb previously approved these changes Sep 27, 2023
Copy link
Contributor

@gozineb gozineb left a comment

Choose a reason for hiding this comment

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

APPROVED !!!!!!!!

@mamadoudicko mamadoudicko merged commit ef2bada into main Sep 27, 2023
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.

3 participants