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: Five Most Popular Courses Chart #266

Merged
merged 8 commits into from
Dec 2, 2024

Conversation

piotr-pajak
Copy link
Member

@piotr-pajak piotr-pajak commented Dec 2, 2024

Jira issue(s)

LC-410

Overview

  • Implemented the FiveMostPopularCoursesChart component.
  • Developed the useMediaQuery hook with comprehensive unit test coverage.
  • Added the useAdminStats hook to handle data fetching.

Screenshots

Mobile:
Screenshot 2024-12-02 at 07 56 19

Desktop:
Screenshot 2024-12-02 at 07 56 02

@piotr-pajak piotr-pajak self-assigned this Dec 2, 2024
@piotr-pajak piotr-pajak added the review me 👀 PR is ready to be reviewed label Dec 2, 2024
@piotr-pajak piotr-pajak force-pushed the pp_feat_410_most_popular_courses_chart branch 2 times, most recently from 0ad4457 to cb801a3 Compare December 2, 2024 09:21
Copy link
Collaborator

Choose a reason for hiding this comment

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

Its teacher statistics, not user or admin

Copy link
Collaborator

Choose a reason for hiding this comment

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

what is this file?

Comment on lines 76 to 89
<Customized
component={() => {
return isEmptyChart ? (
<Text
x={0}
textAnchor="middle"
verticalAnchor="middle"
className="fill-primary-950 h5 md:h3 translate-x-1/2 translate-y-1/2"
>
No data available
</Text>
) : null;
}}
/>
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
<Customized
component={() => {
return isEmptyChart ? (
<Text
x={0}
textAnchor="middle"
verticalAnchor="middle"
className="fill-primary-950 h5 md:h3 translate-x-1/2 translate-y-1/2"
>
No data available
</Text>
) : null;
}}
/>
<Customized
component={() =>
isEmptyChart && (
<Text
x={0}
textAnchor="middle"
verticalAnchor="middle"
className="fill-primary-950 h5 md:h3 translate-x-1/2 translate-y-1/2"
>
No data available
</Text>
);
}
/>

what do you think about that?

Comment on lines +10 to +17
export type AtLeastOne<T> = {
[K in keyof T]-?: Pick<T, K> & Partial<Omit<T, K>>;
}[keyof T];

export type ToMediaQueryObjectParam = AtLeastOne<{
minWidth?: number;
maxWidth?: number;
}>;
Copy link
Member

Choose a reason for hiding this comment

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

nice, but probably we can keep it simple with discriminated union

export type ToMediaQueryObjectParam = {
  minWidth: number;
  maxWidth?: number;
} | {
  minWidth?: number;
  maxWidth: number;
};

@piotr-pajak piotr-pajak force-pushed the pp_feat_410_most_popular_courses_chart branch from cb801a3 to baedf2b Compare December 2, 2024 11:10
@piotr-pajak piotr-pajak force-pushed the pp_feat_410_most_popular_courses_chart branch from c03ca3d to f50d55f Compare December 2, 2024 12:44
@piotr-pajak piotr-pajak merged commit 43c0308 into main Dec 2, 2024
6 checks passed
@piotr-pajak piotr-pajak deleted the pp_feat_410_most_popular_courses_chart branch December 2, 2024 12:52
@piotr-pajak piotr-pajak added merge me 💚 PR is ready to be merged and removed review me 👀 PR is ready to be reviewed labels Dec 2, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
merge me 💚 PR is ready to be merged
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants