-
Notifications
You must be signed in to change notification settings - Fork 9
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
Ab test rammeverk #353
base: master
Are you sure you want to change the base?
Ab test rammeverk #353
Changes from all commits
9e8a610
70ee532
00718e4
2cc458c
49154fa
aed0321
0479343
9bde7ef
ceb0995
bfe9c19
b6b70d5
03a2419
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -12,3 +12,4 @@ yarn-error.log | |
/htdocs/ | ||
.vscode | ||
.env | ||
secrets.json |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -13,7 +13,7 @@ | |
"lint-es": "eslint --cache --cache-location '.eslintcache/' --ext .js --ext .jsx --max-warnings=0 src e2e", | ||
"format": "node prettier.js write", | ||
"format-check": "node prettier.js lint", | ||
"start": "razzle start --inspect --inspect-port 9230", | ||
"start": "cross-env GOOGLE_APPLICATION_CREDENTIALS=./secrets.json razzle start --inspect --inspect-port 9230", | ||
"start-without-ssr": "cross-env RAZZLE_DISABLE_SSR=true razzle start", | ||
"start-with-local-graphql": "cross-env LOCAL_GRAPHQL_API=true yarn start", | ||
"start-with-local-graphql-and-article-converter": "cross-env LOCAL_GRAPHQL_API=true RAZZLE_LOCAL_ARTICLE_CONVERTER=true razzle start", | ||
|
@@ -80,6 +80,7 @@ | |
"defined": "1.0.0", | ||
"express": "^4.16.3", | ||
"express-http-proxy": "^1.4.0", | ||
"googleapis": "^39.2.0", | ||
"graphql": "^14.0.2", | ||
"graphql-tag": "^2.10.0", | ||
"helmet": "^3.15.1", | ||
|
@@ -116,6 +117,7 @@ | |
"sass-loader": "^7.1.0", | ||
"serialize-javascript": "^1.5.0", | ||
"source-map-support": "^0.5.9", | ||
"universal-analytics": "^0.4.20", | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Ser ikke at denne er brukt noe sted? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Den skal jeg fjerne. Gammelt rusk som vi glemte å ta ut. |
||
"warning": "^4.0.3", | ||
"webpack-bundle-analyzer": "^3.0.4" | ||
}, | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,28 +1,89 @@ | ||
import React from 'react'; | ||
import PropTypes from 'prop-types'; | ||
import Modal from '@ndla/modal'; | ||
import { injectT } from '@ndla/i18n'; | ||
import { TopicMenuButton } from '@ndla/ui'; | ||
import { | ||
ExperimentsContext, | ||
Experiment, | ||
Variant, | ||
isValidExperiment, | ||
} from '@ndla/abtest'; | ||
import { Hamburger, Menu } from '@ndla/icons/common'; | ||
|
||
const MastheadMenuModal = ({ children, onMenuExit, t }) => ( | ||
<Modal | ||
size="fullscreen" | ||
activateButton={ | ||
<TopicMenuButton data-testid="masthead-menu-button"> | ||
{t('masthead.menu.title')} | ||
</TopicMenuButton> | ||
} | ||
animation="subtle" | ||
animationDuration={150} | ||
backgroundColor="grey" | ||
noBackdrop | ||
onClose={onMenuExit}> | ||
{children} | ||
</Modal> | ||
const experimentId = 'OtejUgVLRHmGTAGv7jbFyA'; | ||
|
||
const MastheadMenuModal = ({ t, children }) => ( | ||
<ExperimentsContext.Consumer> | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Bruk useContext i stedet |
||
{({ experiments }) => ( | ||
<Modal | ||
size="fullscreen" | ||
onOpen={() => { | ||
if (isValidExperiment({ experiments, id: experimentId })) { | ||
// check if this experimentId exists in data from server | ||
// this event can be tracked by optimize to evaluate variant effectiveness | ||
window.ga('send', { | ||
hitType: 'event', | ||
eventCategory: 'User interactions', | ||
eventAction: 'MenuButtonOpen', | ||
}); | ||
} | ||
}} | ||
activateButton={ | ||
<TopicMenuButton Icon={null} data-testid="masthead-menu-button"> | ||
<Experiment | ||
id={experimentId} | ||
experiments={experiments} | ||
onVariantMount={({ expId, expVar, isActiveExperiment }) => { | ||
if (isActiveExperiment) { | ||
window.ga('set', { expId, expVar }); // Perhaps get GA from ndla/tracker? | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Tracking via GA er kanskje skilt ut i en pakke allerede? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Ja, og abtest-pakken burde vel egentlig vært en del av ndla/tracker og ikke en egen pakke? Så burde man vel kanskje gjøre window.ga-funksjonene i Experiment / Variant komponenten? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Bra innspill. |
||
window.ga('send', { | ||
// report display of experiment variant to analytics/optimize | ||
hitType: 'event', | ||
eventCategory: 'AB test', | ||
eventAction: 'Display variant', | ||
fieldsObject: { | ||
nonInteraction: true, | ||
}, | ||
}); | ||
} | ||
}}> | ||
<Variant variantIndex={0} original> | ||
<Menu /> {t('masthead.menu.title')} orginal | ||
</Variant> | ||
<Variant variantIndex={1}> | ||
<Menu /> {t('abTests.masthead.menu.subjectOverview')} | ||
</Variant> | ||
<Variant variantIndex={2}> | ||
<Menu /> {t('abTests.masthead.menu.overview')} | ||
</Variant> | ||
<Variant variantIndex={3}> | ||
<Menu /> {t('abTests.masthead.menu.topics')} | ||
</Variant> | ||
<Variant variantIndex={4}> | ||
<Hamburger /> {t('masthead.menu.title')} | ||
</Variant> | ||
<Variant variantIndex={5}> | ||
<Hamburger /> {t('abTests.masthead.menu.subjectOverview')} | ||
</Variant> | ||
<Variant variantIndex={6}> | ||
<Hamburger /> {t('abTests.masthead.menu.overview')} | ||
</Variant> | ||
<Variant variantIndex={7}> | ||
<Hamburger /> {t('abTests.masthead.menu.topics')} | ||
</Variant> | ||
</Experiment> | ||
</TopicMenuButton> | ||
} | ||
animation="subtle" | ||
animationDuration={150} | ||
backgroundColor="grey" | ||
noBackdrop> | ||
{children} | ||
</Modal> | ||
)} | ||
</ExperimentsContext.Consumer> | ||
); | ||
|
||
MastheadMenuModal.propTypes = { | ||
onMenuExit: PropTypes.func, | ||
}; | ||
MastheadMenuModal.contentType = ExperimentsContext; | ||
|
||
export default injectT(MastheadMenuModal); |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -7,6 +7,7 @@ | |
*/ | ||
|
||
import React from 'react'; | ||
import { ExperimentsContext } from '@ndla/abtest'; | ||
import WelcomePage from './containers/WelcomePage/WelcomePage'; | ||
import ArticlePage from './containers/ArticlePage/ArticlePage'; | ||
import PlainArticlePage from './containers/PlainArticlePage/PlainArticlePage'; | ||
|
@@ -69,5 +70,12 @@ export const routes = [ | |
]; | ||
|
||
export default function(initialProps = {}, locale) { | ||
return <App initialProps={initialProps} locale={locale} />; | ||
return ( | ||
<ExperimentsContext.Provider | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Bruken av ExperimentsContext er litt forvirrende. Nå brukes samme Context til to foskjellige providers, det virker ikke helt riktig. Ville definert en egen context her og importert den i stedet |
||
value={{ | ||
experiments: initialProps.experiments, | ||
}}> | ||
<App initialProps={initialProps} locale={locale} /> | ||
</ExperimentsContext.Provider> | ||
); | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -27,8 +27,17 @@ const Document = ({ helmet, assets, data }) => { | |
rel="stylesheet" | ||
href="https://fonts.googleapis.com/css?family=Source+Sans+Pro:300,400,400i,600,700|Source+Serif+Pro:400,700" | ||
/> | ||
{config.gaTrackingId && ( | ||
<script async src="https://www.google-analytics.com/analytics.js" /> | ||
{!config.gaTrackingId && ( | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Skal det være '!'? |
||
<script | ||
dangerouslySetInnerHTML={{ | ||
// GA bør sikkert håndteres på en annen måte, via helper eller lib etc | ||
__html: `(function(i,s,o,g,r,a,m){i['GoogleAnalyticsObject']=r;i[r]=i[r]||function(){ | ||
(i[r].q=i[r].q||[]).push(arguments)},i[r].l=1*new Date();a=s.createElement(o), | ||
m=s.getElementsByTagName(o)[0];a.async=1;a.src=g;m.parentNode.insertBefore(a,m) | ||
})(window,document,'script','https://www.google-analytics.com/analytics.js','ga'); | ||
window.ga('create', 'UA-74405776-4', 'auto');`, | ||
}} | ||
/> | ||
)} | ||
<GoogleTagMangerScript /> | ||
{helmet.title.toComponent()} | ||
|
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.
Denne env variabelen bør sikkert settes på en annen måte
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.
Hvor brukes denne?
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.
Denne brukes til å sette client og server secrets for autentisering mot Google analytics management API. Optimalt bør disse verdiene settes som env variabler i de forskjellige miljøene slik at vi slipper secrets.json