-
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: skills quiz v2 enhancements #919
Conversation
18899b6
to
8abea13
Compare
This comment was marked as outdated.
This comment was marked as outdated.
7e034d7
to
f9ca83e
Compare
f9ca83e
to
112aef2
Compare
useEffect(() => { | ||
const fetchLearnerCourseEnrollments = async () => { | ||
try { | ||
const response = await fetchCourseEnrollments(); |
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] Is this fetchCourseEnrollments
function returning all enrollments for the learner across all enterprises? Should we be referring to only the course enrollments relevant for the currently viewed enterprise customer, instead of all enrollments?
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 Yes, the fetchCourseEnrollments
returns a list of all course enrollments for a user.
In Skills Quiz V1, we utilized this function to fetch the list of all enrollments and for consistency, I replicated the same approach in V2 as well.
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 While this isn't blocking feedback, I still might recommend verifying with the team / Product whether this should be using all course enrollments vs. just the course enrollments associated with the specific enterprise customer. The Enterprise Learner Portal generally isn't supposed to be aware of B2C enrollments.
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, though I defer to your team for the final approval.
Jira Ticket: https://2u-internal.atlassian.net/browse/ENT-8191
Description: Added skills quiz v2 enhancements mentioned in the ticket and fixes for the style regressions caused in the application highlighted by @adamstankiewicz here
For all changes
Only if submitting a visual change