-
Notifications
You must be signed in to change notification settings - Fork 3
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: receive unitId as prop and forward to learning-assistant backend #37
feat: receive unitId as prop and forward to learning-assistant backend #37
Conversation
Codecov ReportAttention:
Additional details and impacted files@@ Coverage Diff @@
## main #37 +/- ##
==========================================
- Coverage 81.34% 79.89% -1.45%
==========================================
Files 13 13
Lines 193 199 +6
Branches 24 24
==========================================
+ Hits 157 159 +2
- Misses 34 38 +4
Partials 2 2 ☔ View full report in Codecov by Sentry. |
31db568
to
e90bebc
Compare
This commit modifies the Xpert components to accept a new unitId prop, which presents the unit usage key in which the Xpert is being rendered. This unit ID is forwarded to the chat completion backend API.
e90bebc
to
57f7816
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.
This looks good to me, but I do wonder if it would be better to use React context as opposed to passing the props all the way down. I don't know if context is overkill, but just wanted to throw it out there as an idea.
I think that's a good point. I actively didn't like all of this prop drilling as I was doing it but also didn't really want to deal with setting up the context. I'll take a look at the context API and re-familiarize myself with it to see if that's something I could do. I think that would definitely be an improvement. |
This commit refactors the Disclosure component to receive the MessageForm component as a child prop. This avoids the need to pass the courseId and unitId props through to the Disclosure component, which does not make use of either prop. This decreases the prop drilling and only passes these props through to components that actively use them.
@alangsto Hey, Alie. Following up on your feedback to use context, I did some reading. I feel like using React context isn't justified because we’re only passing the state through a few levels, and that’s considered normal and expected; here's a link to the React docs that explain this rationale in more detail. I did do some refactoring based on the Let me know what you think. |
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 looks good! I appreciate you taking the time to look into using context, and this looks like the right approach.
Description
Jira: COSMO-75
This commit modifies the
Xpert
components to accept a newunitId
prop, which presents the unit usage key in which theXpert
is being rendered. This unit ID is forwarded to the chat completion backend API.