-
Notifications
You must be signed in to change notification settings - Fork 19
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: Updated course cards design #863
Conversation
82a2b23
to
ff5b20c
Compare
Codecov ReportAttention:
Additional details and impacted files@@ Coverage Diff @@
## master #863 +/- ##
==========================================
+ Coverage 84.87% 85.13% +0.26%
==========================================
Files 320 325 +5
Lines 6399 6760 +361
Branches 1552 1641 +89
==========================================
+ Hits 5431 5755 +324
- Misses 941 974 +33
- Partials 27 31 +4 ☔ View full report in Codecov by Sentry. |
3538f2c
to
c90764f
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.
@mahamakifdar19 Can you also add screenshots for other course card variants like "Non-Exec-Ed" courses with statuses like "In Progress", "Upcoming", "Completed" etc.
src/components/dashboard/main-content/course-enrollments/course-cards/AssignedCourseCard.jsx
Outdated
Show resolved
Hide resolved
src/components/dashboard/main-content/course-enrollments/course-cards/styles/_CourseCard.scss
Outdated
Show resolved
Hide resolved
src/components/dashboard/main-content/course-enrollments/course-cards/BaseCourseCard.jsx
Outdated
Show resolved
Hide resolved
6bde2fd
to
5be57e1
Compare
@saleem-latif done! |
src/components/dashboard/main-content/course-enrollments/course-cards/AssignedCourseCard.jsx
Outdated
Show resolved
Hide resolved
src/components/dashboard/main-content/course-enrollments/course-cards/BaseCourseCard.jsx
Outdated
Show resolved
Hide resolved
src/components/dashboard/main-content/course-enrollments/course-cards/BaseCourseCard.jsx
Outdated
Show resolved
Hide resolved
...components/dashboard/main-content/course-enrollments/course-cards/ContinueLearningButton.jsx
Outdated
Show resolved
Hide resolved
src/components/dashboard/main-content/course-enrollments/course-cards/BaseCourseCard.jsx
Outdated
Show resolved
Hide resolved
src/components/dashboard/main-content/course-enrollments/course-cards/BaseCourseCard.jsx
Outdated
Show resolved
Hide resolved
src/components/dashboard/main-content/course-enrollments/course-cards/BaseCourseCard.jsx
Outdated
Show resolved
Hide resolved
src/components/dashboard/main-content/course-enrollments/course-cards/BaseCourseCard.jsx
Outdated
Show resolved
Hide resolved
src/components/dashboard/main-content/course-enrollments/course-cards/styles/_CourseCard.scss
Outdated
Show resolved
Hide resolved
src/components/dashboard/main-content/course-enrollments/course-cards/styles/_CourseCard.scss
Outdated
Show resolved
Hide resolved
9a91068
to
fbe92de
Compare
fbe92de
to
f07d59d
Compare
f07d59d
to
34e2f47
Compare
src/components/dashboard/main-content/course-enrollments/course-cards/BaseCourseCard.jsx
Outdated
Show resolved
Hide resolved
src/components/dashboard/main-content/course-enrollments/course-cards/BaseCourseCard.jsx
Outdated
Show resolved
Hide resolved
src/components/dashboard/main-content/course-enrollments/course-cards/BaseCourseCard.jsx
Outdated
Show resolved
Hide resolved
src/components/dashboard/main-content/course-enrollments/course-cards/BaseCourseCard.jsx
Show resolved
Hide resolved
if (isExecutiveEducation2UCourse && !isCourseStarted() && startDate) { | ||
const formattedStartDate = dayjs(startDate).format('MMM D, YYYY'); | ||
return `Available on ${formattedStartDate}`; | ||
// resumeCourseRunUrl indicates that learner has made progress, available only if the learner has started learning. |
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.
[curious/clarification/] Does the enterprise-course-enrollments
API endpoint return a resume_course_run_url
about its enrollments? For example, while testing, I ensured one of my enrolled courses was not yet started where it said "Start course". Then, I completed at least 1 unit such that the courseware said "Resume course" instead of "Start course" on the courseware home, indicating the course was indeed started / in progress. However, the dashboard course card CTA remains as "Start course" when I believe I would have expected it to say "Resume" at this point.
That said, I think the expectation of having a resumeCourseRunUrl
in the existing, prior implementation on master
may have been flawed?
Out of curiosity, how did you get the course cards to say "Resume" for the screenshots in PR description?
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.
@adamstankiewicz I understood from reviewing the docs and comments that the resumeCourseRunUrl
is returned if the learner has made progress.
However, I'm not entirely certain if this is still the case with the enterprise-course-enrollments API endpoint...
That said, if the resumeCourseRunUrl isn't returned by the API endpoint anymore, do you have any idea how we can determine if the learner has made progress?
One approach I'm considering is to include a condition where if the course has started and the courseRunUrl
exists, we'll view that as indicating progress in learning. Your thoughts?
For testing purposes, I've mocked the data in the enterprise-course-enrollments API response, including the resumeCourseRunUrl field and I was able to get the cards display "Resume" 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.
I understood from reviewing the docs and comments that the resumeCourseRunUrl is returned if the learner has made progress. However, I'm not entirely certain if this is still the case with the enterprise-course-enrollments API endpoint...
Yeah, I'm also a bit confused why some of the code in this app assumes the resumeCourseRunUrl
is there, and documents it. I can't seem to find any history of that field existing in the enterprise-course-enrollments
API response either... very strange. According to Git blame, it appears resumeCourseRunUrl
has been present within the MFE since around February 14, 2020. Checking out an older commit edx-enterprise
from around the same time as it was introduced in this MFE, I can't seem to find any notion of resume_course_run_url
being returned by enterprise-course-enrollments
either.
That said, if the resumeCourseRunUrl isn't returned by the API endpoint anymore, do you have any idea how we can determine if the learner has made progress? One approach I'm considering is to include a condition where if the course has started and the courseRunUrl exists, we'll view that as indicating progress in learning. Your thoughts?
Without any additional data provided by the API, I think your suggestion that's the best we could do given the data the app has to work with, though it's not quite semantically correct (course may be started, without the learning having taken action yet).
Searching for resume_course_run_url
in edx-platform, I came across how resume course URLs are generated for B2C learners (e.g., on home.edx.org) here with get_resume_urls_for_course_enrollments
, where given a user and a list of course enrollments, it returns a list of resume_course_urls
. Makes use of get_key_to_last_completed_block
.
If we need to differentiate between "course has started" and "learner has started", perhaps the enterprise-course-enrollments
API could somehow implement/re-use a similar function such that resumeCourseRunUrl
on enterprise enrollments is no longer undefined
?
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.
@adamstankiewicz I agree with you!
For now, I have removed the usage of resumeCourseRunUrl
and reverted things back to their previous state with the current available data. If the course has started, we display the resume button just as it was before.
However, we do need to differentiate between 'course has started' and 'learner has started' but implementing a logic that returns 'resume_course_run_url' from the enterprise-course-enrollments API seems time consuming and many tickets are already dependent on this one. Perhaps we can create a separate ticket in the next sprint to address this change. What are your thoughts?
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.
Perhaps we can create a separate ticket in the next sprint to address this change. What are your thoughts?
Completely agree! 😄 Making support for resume_course_run_url
is definitely its own chunk of work and can be deferred from this PR/ticket.
[inform] Just dropped a ✅ on the PR
src/components/dashboard/main-content/course-enrollments/course-cards/styles/_CourseCard.scss
Outdated
Show resolved
Hide resolved
src/components/dashboard/main-content/course-enrollments/course-cards/BaseCourseCard.jsx
Outdated
Show resolved
Hide resolved
34e2f47
to
cee8291
Compare
df90a25
to
6f1ba0b
Compare
feat: updated course cards design improvements fix: removed the usage of resumeCourseRunUrl
6f1ba0b
to
bbb53d9
Compare
Jira Ticket: https://2u-internal.atlassian.net/browse/ENT-7843
Figma Designs: https://www.figma.com/file/8da9TyEpEzgxQld3DZE67w/LC2---Top-down-Assignment?type=design&node-id=2620%3A103165&mode=design&t=fIDwm0YtvXr5FUcy-1
Description: Updated all course card designs (Exec-ed and Non Exec-ed) as per figma designs.
UI Screenshots:
For all changes
Only if submitting a visual change