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: create supervisor component #21

Merged
merged 16 commits into from
Dec 19, 2024
Merged

Conversation

mejsiejdev
Copy link
Member

@mejsiejdev mejsiejdev commented Dec 7, 2024

Depends on #23

Adds a supervisor component.

@mejsiejdev mejsiejdev linked an issue Dec 7, 2024 that may be closed by this pull request
@mejsiejdev mejsiejdev force-pushed the 14-create-supervisor-component branch 2 times, most recently from 28cd685 to 1680903 Compare December 8, 2024 19:08
@mejsiejdev mejsiejdev self-assigned this Dec 8, 2024
@mejsiejdev mejsiejdev marked this pull request as ready for review December 8, 2024 19:18
@chewmanji
Copy link
Member

chewmanji commented Dec 8, 2024

Well, I think we need to refactor the supervisor component in design first and then implement it on frontend.
Meanwhile may you start implementing saving conversation to local storage or migrate prompt input to ProseMirror lib?
I assigned myself to the second one but for me there's no difference which one I'd do.

@mejsiejdev
Copy link
Member Author

mejsiejdev commented Dec 8, 2024

Well, I think we need to refactor the supervisor component in design first and then implement it on frontend.
Meanwhile may you start implementing saving conversation to local storage or migrate prompt input to ProseMirror lib?
I assigned myself to the second one but for me there's no difference which one I'd do.

Damn, I wish it was mentioned in the issue that the design is going to be changed 🙃

I will start working on saving conversations.

@chewmanji
Copy link
Member

😭😭
My bad, forgot to change it. Sorry

@mejsiejdev mejsiejdev force-pushed the 14-create-supervisor-component branch from 9a26830 to 24c1833 Compare December 10, 2024 13:00
@mejsiejdev mejsiejdev changed the title Create supervisor component feat: create supervisor component Dec 10, 2024
@mejsiejdev mejsiejdev added the enhancement New feature or request label Dec 10, 2024
Copy link
Member

@maciejkrol18 maciejkrol18 left a comment

Choose a reason for hiding this comment

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

issue (responsiveness): expanding the accordion on mobile causes <body> overflow

suggestion: This whole component isn't bad and we can use it after we address my comments, but we're already using shadcn/ui components throughout our app, and I don't see why we wouldn't use shadcn's accordion (with adjusted code to match our design of course) for this case as well. We would benefit from nice animations and it would ensure that this part of the app is accessible.

<div
className={cn(
"inline-flex w-full flex-col gap-4 rounded-2xl p-4",
(highlight ?? false)
Copy link
Member

Choose a reason for hiding this comment

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

issue (non-blocking): This highlight doesn't make any sense. I don't think there's any reason to pass a highlight prop to this component. Though admittedly this can't be really deduced from our figma design on first glance, I think it makes most sense to highlight the currently active/expanded accordion - thing is, in the current implementation we can have multiple expanded accordions at once

Copy link
Member Author

Choose a reason for hiding this comment

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

the design did not really show that the expanded accordion is highlighted - I thought that only the first one is highlighted, will change that

Comment on lines 42 to 51
<Button
size="icon"
variant="transparent"
title="Rozwiń"
onClick={() => {
setIsOpen(!isOpen);
}}
>
{isOpen ? <ChevronUp size={36} /> : <ChevronDown size={36} />}
</Button>
Copy link
Member

Choose a reason for hiding this comment

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

issue (non-blocking): Having to specifically click this small button to toggle the accordion is bad UX. I suggest making the div with the professor's name and faculty the actual button

Copy link
Member Author

Choose a reason for hiding this comment

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

will change that

@@ -20,6 +20,7 @@ export default {
"chat-bot": "#1D2150",
"chat-user": "#34386A",
"chat-background": "#060C28",
"message-primary": "#5D70B8",
Copy link
Member

Choose a reason for hiding this comment

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

suggestion: Currently, message-primary is used for the chat bot's message, yet we also have a chat-bot variable. This appears to be caused by the fact that on our homepage, chat-bot is used as the bot's message background color. If we stay with message-primary (see below), I think the homepage could be adjusted so that it represents how the chat actually looks (bot message uses the brighter message-primary as its background color)

Though on another hand, this color doesn't pass WCAG AAA contrast check given the white foreground (https://webaim.org/resources/contrastchecker/), while chat-bot passes all checks

Copy link
Member Author

Choose a reason for hiding this comment

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

I used this color because it was used in the design, we have to choose another color for it then

Copy link
Member

Choose a reason for hiding this comment

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

I understand, that's not a mistake on your part. We'll address this design problem later

<div className="flex items-center justify-between gap-4">
<div className="flex flex-col gap-1">
<p className="text-xl font-bold">{name}</p>
<p>{faculty}</p>
Copy link
Member

Choose a reason for hiding this comment

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

thought: Since the API returns faculties in english, we will probably have to setup some sort of a "dictionary" within the app, which will have english-polish faculty name pairs that we will then use in api requests and the ui respectively

@mejsiejdev
Copy link
Member Author

Hey @maciejkrol18 , can you take a look at it again? It is now using the accordion from shadcn

Copy link
Member

@maciejkrol18 maciejkrol18 left a comment

Choose a reason for hiding this comment

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

Good job, everything looks and works nice. I'll let you do the honors of merging

edit: and the honors of resolving conflicts :trollface:

Comment on lines +10 to +14
<PromochatorIcon
imageWidth={36}
imageHeight={36}
imageClassName="py-2 px-1"
/>
Copy link
Member

Choose a reason for hiding this comment

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

note: This icon gets really tiny on mobile. Just noting this since this whole markup is just for showcasing the component, this should be handled in #45

@@ -20,6 +20,7 @@ export default {
"chat-bot": "#1D2150",
"chat-user": "#34386A",
"chat-background": "#060C28",
"message-primary": "#5D70B8",
Copy link
Member

Choose a reason for hiding this comment

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

I understand, that's not a mistake on your part. We'll address this design problem later

@mejsiejdev mejsiejdev merged commit ccbf982 into main Dec 19, 2024
1 check passed
@mejsiejdev mejsiejdev deleted the 14-create-supervisor-component branch December 19, 2024 16:33
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Create supervisor component
3 participants