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. |
JodyBaileyy
force-pushed
the
jodybaileyy/recommendations-experiment-implementation
branch
from
cdb34db
to
4790bf4
Compare
src/experimentContext.jsx
Outdated
|
|
||
| export const useCountryCode = (setCountryCode) => { | ||
| React.useEffect(() => { | ||
| let isMounted = true; |
There was a problem hiding this comment.
Why is this important?
There was a problem hiding this comment.
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.
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.
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.
+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.
My one request here is that this file get renamed to ExperimentContext, as is exporting a component provider.
JodyBaileyy
deleted the
jodybaileyy/recommendations-experiment-implementation
branch
Description
recommendations_contextendpoint which gathers the user's country codeexperimentContextwhich 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.courseRunKeywhich comes from ourproduct_recommendationsendpoint enhancementJIRA Ticket
PTECH-3282