-
Notifications
You must be signed in to change notification settings - Fork 85
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
✨(frontend) dashboard teacher: page profile courses #1951
Conversation
5f25a8f
to
22b9ec7
Compare
22b9ec7
to
fb0d2ac
Compare
678e1de
to
88c4ec6
Compare
src/frontend/js/widgets/Dashboard/components/TeacherCourseSearchFilters/_styles.scss
Show resolved
Hide resolved
src/frontend/js/widgets/Dashboard/components/TeacherCourseSearchFilters/index.tsx
Outdated
Show resolved
Hide resolved
src/frontend/js/widgets/Dashboard/components/TeacherCourseSearchFilters/index.tsx
Show resolved
Hide resolved
src/frontend/js/widgets/Dashboard/components/TeacherCourseSearchFilters/index.tsx
Outdated
Show resolved
Hide resolved
6bc9958
to
185555d
Compare
14aa15b
to
a9e52c8
Compare
d0574c7
to
cfab391
Compare
1672b03
to
207ddb5
Compare
src/frontend/js/widgets/Dashboard/components/DashboardItem/stories.mock.ts
Outdated
Show resolved
Hide resolved
Great work ! Just a general feeback : for next reviews, try to split our changes into smaller commit, this PR was a bit hard to review 💊 |
207ddb5
to
e1f0a53
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Loading hook and animation have been moved into #1971
src/frontend/js/widgets/Dashboard/components/DashboardItem/stories.mock.ts
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Last one review then we will be ready to merge
We need Course to contain lot's more information for our course endpoint that will be make for teacher dashboard course page.
e1f0a53
to
326c1f7
Compare
status: CourseStatusFilter; | ||
type: CourseTypeFilter; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should be optional, nope ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As is, we always have a filter value with "all"
instead of undefined
for all the results
</span> | ||
</Spinner> | ||
)} | ||
{!fetching && courses.length && ( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Currently, if courses array is empty, 0
will be displayed.
{!fetching && courses.length && ( | |
{!fetching && courses.length > 0 && ( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think a test is missing to check what happens when courses is empty.
d9335e9
to
b6a0e73
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM ✅
Part of teacher dashboard development, list teacher courses.
b6a0e73
to
a56f322
Compare
Purpose
Add course search page for teacher profile in new teacher dashboard.
Sketch preview
Proposal
Joanie.Course
intoJoanie.CourseLight
Joanie.Course
used in Teacher Dashboard listingsCourseGlimpse
andCourseGlimpseList
in newTeacherDashboardCourse
page.TeacherDashboardCourse
page.Todo
Backend filters for courses
Date manipulation
it would be nice to have a library to handle date transformations and formating.
The one used on my previous project was date-fns.