Skip to content

Conversation

moalidv
Copy link
Collaborator

@moalidv moalidv commented Sep 16, 2025

Quality Checklist

  • I took the time to compare my implementation with the design.
  • I ensured that the this pull request satisfies all the requirements in the associated task.
  • I performed a self-review.

Links

Important

Incase you have any valuable knowledge that you think that it is worth writing down, feel free to add it to our handbook. Wounder why we are doing this? check this guide.

@moalidv moalidv self-assigned this Sep 16, 2025
@neuodev
Copy link
Member

neuodev commented Sep 16, 2025

Warnings
⚠️ No clickup task found in this pull request

Generated by 🚫 dangerJS against dbaff00

@neuodev
Copy link
Member

neuodev commented Sep 16, 2025

🚀 WEB
🚀 UI
🚀 LANDING
🚀 DASHBOARD

@moalidv moalidv force-pushed the mo/settings-info-tab branch from 8c329ef to ba49849 Compare September 16, 2025 17:23
@neuodev
Copy link
Member

neuodev commented Sep 16, 2025

🚀 WEB
🚀 UI
🚀 LANDING
🚀 DASHBOARD

@moalidv moalidv requested a review from mmoehabb September 16, 2025 17:27
"tutor-settings.topics.selection-dialog.title": "شارك خبراتك لتجذب الطلاب الذين يشاركونك الشغف!",
"tutor-settings.topics.selection-dialog.description": "حدد المجالات التي تبرع فيها أو تهتم بها، مثل الرياضة أو التكنولوجيا أو علم النفس، لنساعدك في تخصيص تجربة تدريسية تناسب اهتمامات طلابك وتزيد من تفاعلهم وحماسهم للتعلم.",
"student-settings.add-topics-button.label": "قم بأضافة الموضوعات المفضلة الخاصة بك",
"student-settings.updated-successfully": "تم تحديث بيانانك بنجاح",
Copy link
Collaborator

Choose a reason for hiding this comment

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

"تم تحديث بياناتك بنجاح."

Comment on lines 70 to 73
topics: [],
career: "",
level: IStudent.EnglishLevel.Beginner,
aim: "",
Copy link
Collaborator

Choose a reason for hiding this comment

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

The defaults should be the actual student data.

Write mock hooks if you needed to.

Copy link
Collaborator

Choose a reason for hiding this comment

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

This is how it looks on Galaxy S10:
Screenshot_20250916_211355-1

  1. Try to make the tabs view more responsive.
  2. The font referred to in the screenshot is quite big.

@moalidv moalidv force-pushed the mo/settings-info-tab branch 4 times, most recently from 1178d02 to f829bad Compare September 17, 2025 15:44
@neuodev
Copy link
Member

neuodev commented Sep 17, 2025

🚀 WEB
🚀 UI
🚀 LANDING
🚀 DASHBOARD

@neuodev
Copy link
Member

neuodev commented Sep 17, 2025

🚀 WEB
🚀 UI
🚀 LANDING
🚀 DASHBOARD

@moalidv moalidv force-pushed the mo/settings-info-tab branch from f829bad to 0a9cbd3 Compare September 17, 2025 15:53
@moalidv moalidv requested a review from mmoehabb September 17, 2025 15:56
@moalidv moalidv force-pushed the mo/settings-info-tab branch 2 times, most recently from 8aa855b to 30692e4 Compare September 18, 2025 06:01
@neuodev
Copy link
Member

neuodev commented Sep 18, 2025

🚀 WEB
🚀 UI
🚀 LANDING
🚀 DASHBOARD

Comment on lines 94 to 125
const levelValues = Object.values(IStudent.EnglishLevel).filter(
(value) => typeof value === "number"
);

function getRandomLevel() {
const randomLevel = sample(levelValues);
return randomLevel as IStudent.EnglishLevel;
}

export function useStudentMeta(): UseQueryResult<{
career: string;
level: IStudent.EnglishLevel;
aim: string;
}> {
const find = useCallback(async () => {
return {
career: sample([
"labels.jobs.student",
"labels.jobs.tutor",
"labels.jobs.doctor",
"labels.jobs.police-officer",
]),
level: getRandomLevel(),
aim: "labels.random-aim",
};
}, []);

return useQuery({
queryFn: find,
queryKey: ["find-student-meta"],
});
}
Copy link
Collaborator

Choose a reason for hiding this comment

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

First of all... no need for randomness for these mock functions/hooks. Second, I don't think we need to mock this.
You can just implement it as the mocking part falls on the server responsibility.

export function useFindStudentById(id): UseQueryResult<IStudent.FindApiResponse> {
  const api = useApi();
  const find = useCallback(async () => {
    return api.student.findById(...);
  }, [api.student]);

  return useQuery({
    queryFn: find,
    queryKey: ["find-student-by-id"],
  });
}

@moalidv moalidv force-pushed the mo/settings-info-tab branch 3 times, most recently from 1d51e4f to 6afcef8 Compare September 21, 2025 15:35
@moalidv moalidv requested a review from mmoehabb September 21, 2025 15:35
@neuodev
Copy link
Member

neuodev commented Sep 21, 2025

🚀 WEB
🚀 UI
🚀 LANDING
🚀 DASHBOARD

@neuodev
Copy link
Member

neuodev commented Sep 21, 2025

🚀 WEB
🚀 UI
🚀 LANDING
🚀 DASHBOARD


return useQuery({
queryFn: find,
queryKey: ["find-student-by-id"],
Copy link
Collaborator

Choose a reason for hiding this comment

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

Use QueryKey.


return useMutation({
mutationFn: update,
mutationKey: ["update-user-topics"],
Copy link
Collaborator

Choose a reason for hiding this comment

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

Use MutationKey.

const form = useForm<IForm>({
defaults: {
topics: studentTopics.data || [],
career: studentData?.jobTitle || intl("labels.jobs.student"),
Copy link
Collaborator

Choose a reason for hiding this comment

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

or empty string is better. Don't force values or auto-fill user inputs.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Great work so far.
It requires more effort from us in order to make it more user friendly. As shown in the attached screenshot. There is no enough spaces between the buttons.

We may make it more responsive by breaking the label into two lines when the width gets this small. WDYT?
Screenshot_20250922_110953

@moalidv moalidv force-pushed the mo/settings-info-tab branch from 6afcef8 to d1c9746 Compare September 22, 2025 16:45
@moalidv moalidv requested a review from mmoehabb September 22, 2025 16:45
@neuodev
Copy link
Member

neuodev commented Sep 22, 2025

🚀 WEB
🚀 UI
🚀 LANDING
🚀 DASHBOARD

@moalidv moalidv force-pushed the mo/settings-info-tab branch from d1c9746 to aabd992 Compare September 23, 2025 17:23
@neuodev
Copy link
Member

neuodev commented Sep 23, 2025

🚀 WEB
🚀 UI
🚀 LANDING
🚀 DASHBOARD

@moalidv moalidv force-pushed the mo/settings-info-tab branch from aabd992 to 50687f2 Compare September 24, 2025 12:06
@neuodev
Copy link
Member

neuodev commented Sep 24, 2025

🚀 WEB
🚀 UI
🚀 LANDING
🚀 DASHBOARD

Copy link
Collaborator

@mmoehabb mmoehabb left a comment

Choose a reason for hiding this comment

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

Please test the implementation and ensure that it's working as expected.

@moalidv moalidv force-pushed the mo/settings-info-tab branch from 50687f2 to 133b2f8 Compare September 29, 2025 14:19
@neuodev
Copy link
Member

neuodev commented Sep 29, 2025

🚀 WEB
🚀 UI
🚀 LANDING
🚀 DASHBOARD

@mmoehabb mmoehabb force-pushed the mo/settings-info-tab branch from 133b2f8 to dbaff00 Compare September 29, 2025 15:15
@neuodev
Copy link
Member

neuodev commented Sep 29, 2025

🚀 WEB
🚀 UI
🚀 LANDING
🚀 DASHBOARD

@moalidv moalidv requested a review from mmoehabb October 5, 2025 19:16
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.

3 participants