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(nova): implemented tutor ratings #224

Merged
merged 2 commits into from
Dec 22, 2024
Merged

feat(nova): implemented tutor ratings #224

merged 2 commits into from
Dec 22, 2024

Conversation

mostafakamar2308
Copy link
Member

No description provided.

@neuodev
Copy link
Member

neuodev commented Dec 17, 2024

🚀 Nova
🚀 Luna
🚀 Nebula
🚀 Dashboard

@neuodev
Copy link
Member

neuodev commented Dec 17, 2024

🚀 Nova
🚀 Luna
🚀 Nebula
🚀 Dashboard

const intl = useFormatMessage();
return (
<div className="h-full flex flex-col justify-center items-center gap-10 mt-20">
<div className="w-[80px] h-[80px] animate-spin flex items-center relative rounded-full justify-center bg-[conic-gradient(from_180deg_at_50%_50%,#1D7C4E_0deg,rgba(17,173,207,0)_360deg)]">
Copy link
Member

Choose a reason for hiding this comment

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

Why do we have to do it this way?

Copy link
Member Author

Choose a reason for hiding this comment

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

When I tried to copy as svg it wasn't being copied correctly, and I tried multiple times
So, I thought it won't be hard to recreate it using HTML

Copy link
Member

Choose a reason for hiding this comment

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

The spinner should be resuable. Please move it to luna and add storybooks.

One last thing, the spinning direction not correct.

Copy link
Member Author

Choose a reason for hiding this comment

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

This was a new style of loading so I thought it was a one off thing.
When I saw it reused in the dashboard, I made it reusable in luna
But I've done it again with the error here.

<Typography
element="h4"
weight="bold"
className="text-natural-950 text-center mb-20"
Copy link
Member

Choose a reason for hiding this comment

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

It works I know but this is not the right place to put mb-20. I will let you decide where to put it.

if (query.isLoading) return <Loading className="h-60" />;
if (query.isError) return <h1>Error</h1>;
if (!query.data) return null;
const ratings = ratingOrganizer(ratingsQuery.data.list, user.id);
Copy link
Member

Choose a reason for hiding this comment

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

For function names, it is better to use verbs. In this case it is better to name it orgainzeRatings or groupRatings.

<Typography
element="subtitle-1"
weight="medium"
className="text-natural-950 w-2/3 text-center"
Copy link
Member

Choose a reason for hiding this comment

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

Why do we have to assing w-2/3?

Copy link
Member Author

@mostafakamar2308 mostafakamar2308 Dec 17, 2024

Choose a reason for hiding this comment

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

wanted to have the sentence on 2 lines
image

I will find better way. I think it's better to have it on 2 lines and not let it stretch to be easier to read

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 divided the text in ar-eg. I think this is a more elegant solution

Copy link
Member

Choose a reason for hiding this comment

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

It should be one sentence

@@ -7,6 +7,7 @@ import { LocalId } from "@litespace/luna/locales";
import cn from "classnames";
import { AnimatePresence, motion } from "framer-motion";
import ProfileInfo from "./ProfileInfo";
import Ratings from "./Ratings";
Copy link
Member

Choose a reason for hiding this comment

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

Remember to always use absolute paths. You can always spot this by maing a self-review once you opend the pull request.

@@ -114,7 +115,7 @@ export const TutorTabs: React.FC<{
{tab === "ratings" ? (
<Animate key="ratings" tab="ratings">
<div className="min-h-96">
Copy link
Member

Choose a reason for hiding this comment

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

Remove this wrapping div. It was just for testing purposes.

Comment on lines 10 to 17
ratings: Array<{
name: string | null;

userId: number;

imageUrl: string | null;
}>;
value: number;
Copy link
Member

Choose a reason for hiding this comment

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

This type is the same as the TutorRatingCardGroup props. Please use this type and don't ceate a new one.

@neuodev
Copy link
Member

neuodev commented Dec 17, 2024

🚀 Nova
🚀 Luna
🚀 Nebula
🚀 Dashboard

@neuodev
Copy link
Member

neuodev commented Dec 17, 2024

🚀 Nova
🚀 Luna
🚀 Nebula
🚀 Dashboard

@neuodev
Copy link
Member

neuodev commented Dec 19, 2024

@mostafakamar2308 please resolve the conflicts.

@neuodev
Copy link
Member

neuodev commented Dec 19, 2024

🚀 Nova
🚀 Luna
🚀 Nebula
🚀 Dashboard

@mostafakamar2308
Copy link
Member Author

@neuodev Done 🚀

packages/assets/package.json Show resolved Hide resolved
packages/assets/package.json Outdated Show resolved Hide resolved
packages/assets/package.json Outdated Show resolved Hide resolved
packages/assets/package.json Outdated Show resolved Hide resolved
packages/headless/src/rating.ts Outdated Show resolved Hide resolved
packages/luna/src/locales/ar-eg.json Outdated Show resolved Hide resolved
apps/nova/src/components/TutorProfile/Ratings.tsx Outdated Show resolved Hide resolved
apps/nova/src/components/TutorProfile/Ratings.tsx Outdated Show resolved Hide resolved
const intl = useFormatMessage();
return (
<div className="h-full flex flex-col justify-center items-center gap-10 mt-20">
<div className="w-[80px] h-[80px] animate-spin flex items-center relative rounded-full justify-center bg-[conic-gradient(from_180deg_at_50%_50%,#1D7C4E_0deg,rgba(17,173,207,0)_360deg)]">
Copy link
Member

Choose a reason for hiding this comment

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

The spinner should be resuable. Please move it to luna and add storybooks.

One last thing, the spinning direction not correct.

<Typography
element="subtitle-1"
weight="medium"
className="text-natural-950 w-2/3 text-center"
Copy link
Member

Choose a reason for hiding this comment

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

It should be one sentence

@mostafakamar2308 mostafakamar2308 marked this pull request as draft December 20, 2024 06:19
apps/nova/src/components/TutorProfile/Ratings.tsx Outdated Show resolved Hide resolved
Comment on lines +14 to +17
if (rating.userId === currentUserId) {
currentUserRating = rating;
return;
}
Copy link
Member

Choose a reason for hiding this comment

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

I am not quite sure that this logic will work. When you return the logic below it will never be executed.

If you want to find the current user rating, you can find it independently.

const userRating = ratings.find(rating => rating.userId === currentUserId) || null;

Comment on lines +9 to +11
const ratingsWithFeedback: IRating.RateeRatings[] = [];
const ratingsWithoutFeedback: Omit<TutorRatingCardGroupProps, "tutorName">[] =
[];
Copy link
Member

Choose a reason for hiding this comment

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

I think we can do better. Based on the fact that the ratings returned by the backend will be ordered, we can rewrite the logic like this.

const organizedRatings: Array<{
    type: 'owner',
    rating: IRating.RateeRatings, // note: that their is a typo in "RateeRatings". It should be just "RateeRating".
}| {
    type: 'group',
    ratings: IRating.RateeRatings[]; // note: you don't need to use `TutorRatingCardGroupProps` because incase the group has only one member it should be rendered in `TutorRAtingCard` and not `TutorRatingCardGroup`. The `Rating` component will decide this. It is not the role of this function to decide. 
} | {
    type: 'with-feedback',
    rating: IRating.RateeRatings,
}> = []

let buffer: IRating.RateeRatings[] = [];

ratings.reduce((prev, curr) => {
    if (curr.userId === currentUserId) orgainizedRatings.push({type: 'owner', rating: curr});
    if (!prev) return curr;
    // if the current rating is the same as the previous one it should go the buffer.
    if (prev && prev.value === curr.value && !curr.feedback) buffer.push(curr);
    // prev !== curr => end the previous group, and clear the buffer
    if (prev && ((prev.value !== curr.value) || (prev.value == curr.value && !!curr.feedback))) {
        const firstRating = first(buffer)
        const length = buffer.length;

        if (length === 1 && firstRatings && firstRating.feedback) organizedRatings.push({type: 'with-feedback', rating: firstRating});
        if (length > 1) organizedRatings.push({type: 'group', ratings: buffer});

        buffer = [curr] // new group 
    }

    return curr
}, null)

@neuodev
Copy link
Member

neuodev commented Dec 20, 2024

@mostafakamar2308 please implement these (1, 2) designs instead.

@neuodev
Copy link
Member

neuodev commented Dec 20, 2024

🚀 Nova
🚀 Luna
🚀 Nebula
🚀 Dashboard

@mostafakamar2308 mostafakamar2308 marked this pull request as ready for review December 20, 2024 13:44
@neuodev
Copy link
Member

neuodev commented Dec 20, 2024

🚀 Nova
🚀 Luna
🚀 Nebula
🚀 Dashboard

@neuodev
Copy link
Member

neuodev commented Dec 22, 2024

🚀 Nova
🚀 Luna
🚀 Nebula
🚀 Dashboard

import { IRating } from "@litespace/types";
import { TutorRatingCardGroupProps } from "@litespace/luna/TutorFeedback";

export const organizeRatings = (
Copy link
Member

Choose a reason for hiding this comment

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

The implementation doesn't match this.

@neuodev neuodev merged commit 713b804 into master Dec 22, 2024
2 checks passed
@neuodev neuodev deleted the mk/tutor-rating branch December 22, 2024 16:19
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants