-
Notifications
You must be signed in to change notification settings - Fork 80
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: Implemented product recommendations experiment #174
feat: Implemented product recommendations experiment #174
Conversation
Codecov ReportPatch coverage:
Additional details and impacted files@@ Coverage Diff @@
## master #174 +/- ##
==========================================
+ Coverage 96.20% 96.29% +0.09%
==========================================
Files 180 182 +2
Lines 1606 1699 +93
Branches 286 300 +14
==========================================
+ Hits 1545 1636 +91
- Misses 56 58 +2
Partials 5 5
☔ View full report in Codecov by Sentry. |
cdb34db
to
4790bf4
Compare
src/experimentContext.jsx
Outdated
|
||
export const useCountryCode = (setCountryCode) => { | ||
React.useEffect(() => { | ||
let isMounted = true; |
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.
Why is this important?
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.
React.useEffect
should be part of the component. useCountryCode
should just do the fetching in this case. Then useEffect(fn, [])
should run only once. There won't be need for isMounted
.
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.
True, I can't really think of a scenario where the component will be unmounted, I'll adjust it
src/experimentContext.jsx
Outdated
export const useCountryCode = (setCountryCode) => { | ||
React.useEffect(() => { | ||
api | ||
.fetchRecommendationsContext() | ||
.then((response) => { | ||
setCountryCode(response.data.countryCode); | ||
}) | ||
.catch(() => { | ||
setCountryCode(''); | ||
}); | ||
/* eslint-disable */ | ||
}, []); | ||
}; |
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.
Keeping the useEffect
for setting the country code in it's own hook because it makes the ExperimentProvider
less bloated and it's much easier to test
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.
+100%. Honestly, for this, having all of the hooks be as separated as possible is valuable, because they are less for-a-single-purpose. I would almost break the useIsMobile, but is small enough it doesn't make a huge difference.
src/experimentContext.jsx
Outdated
@@ -0,0 +1,64 @@ | |||
import React from 'react'; |
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.
My one request here is that this file get renamed to ExperimentContext, as is exporting a component provider.
Description
recommendations_context
endpoint which gathers the user's country codeexperimentContext
which both makes the api call to fetch the user's country code and stores the stateful values for activating our experiment and which widget container to render based on the variant the user is bucketed into.courseRunKey
which comes from ourproduct_recommendations
endpoint enhancementJIRA Ticket
PTECH-3282