-
Notifications
You must be signed in to change notification settings - Fork 28
feat: implement design for new CTM page #463
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
Conversation
4ac4bea to
7e8fb58
Compare
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## master #463 +/- ##
==========================================
+ Coverage 85.94% 86.00% +0.05%
==========================================
Files 185 197 +12
Lines 3879 4315 +436
Branches 959 1089 +130
==========================================
+ Hits 3334 3711 +377
- Misses 527 582 +55
- Partials 18 22 +4 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
d8747aa to
7d4e522
Compare
Implement design for new Course Team Management Page,
- This commit covers TNL2-310, TNL2-311, TNL2-312, TNL2-315 which offers a course team management table with get request intgeration
and it provides add user to a course, delete user from a course & change role feature for a user in a course.
- This also covers unit tests for above mentioned tickets.
cc89a7f to
862d04a
Compare
This commit covers - TNL2-313 - Changes Confirmation Modal with all edge cases. - TNL2-314 - stop user with a popup from refreshing or navigation when having unsaved changes. - This also includes test coverage for the added code.
0f63c14 to
1609703
Compare
this commit includes - sorting improvement as now manual sorting handles numeric cases "test 5" < "test 15" and it is now case insensitive. - now course table is using course_id as unique key instead of run which wasn't unique as used in earlier commits
d64ecf1 to
3262996
Compare
This commit covers TNL2-316 & TNL2-317, and - adds error modal incase of unsaved changes after update API is called. Which include custom Alert to show the modal. - It also include edge case handling for different cases and also updated the unit tests. - It also show an alert in case of backend error e.g. 500, 400, 401 etc - test coverage.
3262996 to
f2dfd8c
Compare
| courseName, number, runId, from, to, | ||
| }) => ( | ||
| <p key={`role-${runId}`}> | ||
| <span className="font-medium">{`${courseName} (${number} - ${runId}) ${from === 'instructor' ? 'Instructor' : 'Staff'} → ${to === 'instructor' ? 'Instructor' : 'Staff'}`}</span> |
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.
translation for this missing?
| labels={{ | ||
| default: 'Save', | ||
| pending: 'Saving', | ||
| complete: 'Saved', |
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.
translations missing?
| className="course-team-management-course-status-badge" | ||
| variant={value === 'active' ? 'success' : 'light'} | ||
| > | ||
| {value === 'active' |
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.
can we create cconstant file for all hardcoded strings, even for staff instructor,
| const courseId = row.original.course_id; | ||
| const value = rowRoles[courseId]; | ||
| // If role is 'null', default to staff for display only | ||
| const displayValue = value === 'null' ? 'staff' : value; |
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.
is null a string or there is value like that?
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.
Nah, there isn't any 'null' value.
But I am changing null to 'null' str at the start. So that while sorting, 'null' which will be displayed as 'staff' stays together with 'staff' in case of ascending (Admin, null(string), staff) or descending(staff, null(string), admin) order.
Nothing to worry here as we have implemented custom sorting, so it was necessary!
| role="button" | ||
| tabIndex={0} | ||
| onKeyDown={(e) => { | ||
| if (e.key === 'Enter' || e.key === ' ') { |
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.
use trim method, and use length 0 if that works
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.
This doesn't apply here as we are only checking key press here.
| role="button" | ||
| tabIndex={0} | ||
| onKeyDown={(e) => { | ||
| if (e.key === 'Enter' || e.key === ' ') { |
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.
use trim method, and use length 0 if that works
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.
always create named function to reduce memory usage and useCallback should be used
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.
same here, This comment doesn't apply here as we are only checking key press(enter/space) here.
| {org === '' && <Icon src={Check} className="float-end" size="sm" />} | ||
| </div> | ||
| )} | ||
| {filteredChoices.map((choice) => ( |
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.
add filteredChoices.length > 0 condition
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.
This condition isn't required as map can handle empty arrays!
| { value: '', label: messages.allCoursesFilterLabel }, | ||
| { value: 'active', label: messages.activeCoursesFilterLabel }, | ||
| { value: 'archived', label: messages.archivedCoursesFilterLabel }, | ||
| ].map((option) => ( |
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.
create a separate const instead of inline
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 am not a fan of creating a const until it's not used twice.
But yes we can create 1 for this as it's written in multiline.
| {[ | ||
| { value: 'staff', label: messages.statusStaffFilterLabel }, | ||
| { value: 'instructor', label: messages.statusAdminFilterLabel }, | ||
| ].map((option) => ( |
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.
create a const instead of inline
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 am not a fan of creating a const until it's not used twice.
But yes we can create 1 for this as it's written in multiline.
src/userMessages/Alert.jsx
Outdated
| <div role="alert" className="flex-grow-1"> | ||
| {children} | ||
| </div> | ||
| {dismissible && <Button variant="link" size="inline" className="dismiss-button" onClick={onDismiss}>Dismiss</Button>} |
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.
Shoudl we add button translations
a1a0567 to
17baefe
Compare
In this commit, - localized missing strings - add constants for hardcoded values in constants.js file - add test coverage for AlertList
17baefe to
479d5e4
Compare
|
Closing this PR as we have no active maintainers for this repo to approve and merge this PR. |
Ticket: TNL2-136
Implement design for new Course Team Management Page.
Course Team Bulk Management Page Design:
Complete Flow: ⬇️
Screen.Recording.2025-08-14.at.8.28.25.AM.mp4
In case if there are some errors for some course while updating the course roles: ⬇️
Screen.Recording.2025-08-13.at.6.49.05.PM.mov
Error(400, 500 etc) if any from update API: ⬇️
Screen.Recording.2025-08-13.at.5.24.45.PM.mov
Error(400, 500 etc) if any from get API: ⬇️
