Issue mozilla#2807: Move privacyScrollOffset from state and into a co…

…nstant since it never changes; switch ExperimentPage to mount() from shallow() in tests; stop testing component state since it moved into subcomponents and markup should be enough to reflect functionality;
lmorchard · Oct 25, 2017 · a983d55 · a983d55
1 parent 4bc69b3
commit a983d55
Show file tree

Hide file tree

Showing 3 changed files with 41 additions and 32 deletions.
diff --git a/frontend/src/app/containers/ExperimentPage/DetailsHeader.js b/frontend/src/app/containers/ExperimentPage/DetailsHeader.js
@@ -7,6 +7,8 @@ import { experimentL10nId } from '../../lib/utils';
 
 import type { DetailsHeaderProps } from './types';
 
+export const PRIVACY_SCROLL_OFFSET = 15;
+
 export default class DetailsHeader extends React.Component {
   props: DetailsHeaderProps;
 
@@ -17,14 +19,14 @@ export default class DetailsHeader extends React.Component {
       useStickyHeader: false,
       stickyHeaderSiblingHeight: 0,
       changeHeaderOn: 125,
-      privacyScrollOffset: 15
     };
   }
 
   render() {
     const { highlightPrivacy, l10nId } = this;
 
     const {
+      sendToGA,
       userAgent,
       hasAddon,
       progressButtonWidth,
@@ -37,6 +39,7 @@ export default class DetailsHeader extends React.Component {
       surveyURL,
       installExperiment,
       doShowEolDialog,
+      doShowPreFeedbackDialog,
       uninstallExperimentWithSurvey
     } = this.props;
 
@@ -99,6 +102,7 @@ export default class DetailsHeader extends React.Component {
           </header>
           <ExperimentControls
             {...{
+              sendToGA,
               hasAddon,
               userAgent,
               experiment,
@@ -112,14 +116,15 @@ export default class DetailsHeader extends React.Component {
               isDisabling,
               highlightPrivacy,
               doShowEolDialog,
+              doShowPreFeedbackDialog,
               uninstallExperimentWithSurvey
             }}
           />
           <MinimumVersionNotice
-            {...{ userAgent, title, hasAddon, min_release }}
+            {...{ userAgent, title, hasAddon, min_release, sendToGA }}
           />
           <MaximumVersionNotice
-            {...{ userAgent, title, hasAddon, max_release, graduated }}
+            {...{ userAgent, title, hasAddon, max_release, graduated, sendToGA }}
           />
         </LayoutWrapper>
       </div>
@@ -213,7 +218,6 @@ export default class DetailsHeader extends React.Component {
       setScrollY,
       flashMeasurementPanel
     } = this.props;
-    const { privacyScrollOffset } = this.state;
     const detailsHeaderWrapperHeight = getElementOffsetHeight(
       '.details-header-wrapper'
     );
@@ -225,7 +229,7 @@ export default class DetailsHeader extends React.Component {
     evt.preventDefault();
     setScrollY(
       getElementY('.measurements') +
-        (stickyHeaderSiblingHeight - privacyScrollOffset)
+        (stickyHeaderSiblingHeight - PRIVACY_SCROLL_OFFSET)
     );
     this.setState({
       changeHeaderOn,
@@ -266,6 +270,7 @@ export const ExperimentControls = ({
   isDisabling,
   highlightPrivacy,
   doShowEolDialog,
+  doShowPreFeedbackDialog,
   sendToGA
 }) => {
   const {
@@ -287,11 +292,7 @@ export const ExperimentControls = ({
         eventLabel: title
       });
     } else {
-      evt.preventDefault();
-      this.setState({
-        showPreFeedbackDialog: true
-      });
-
+      doShowPreFeedbackDialog(evt);
       sendToGA('event', {
         eventCategory: 'ExperimentDetailsPage Interactions',
         eventAction: 'Give Feedback',
@@ -403,7 +404,7 @@ export const ExperimentControls = ({
         </a>
       </Localized>
       <EnableButton
-        {...{ experiment, installExperiment, isEnabling, progressButtonWidth }}
+        {...{ experiment, installExperiment, isEnabling, progressButtonWidth, sendToGA }}
       />
     </div>
   );
@@ -515,11 +516,12 @@ export const EnableButton = ({
   experiment: { title, web_url, platforms },
   installExperiment,
   isEnabling,
-  progressButtonWidth
+  progressButtonWidth,
+  sendToGA
 }) => {
   const useWebLink = (platforms || []).indexOf('web') !== -1;
   if (useWebLink) {
-    return <WebExperimentControls {...{ web_url, title }} />;
+    return <WebExperimentControls {...{ web_url, title, sendToGA }} />;
   }
 
   return (

diff --git a/frontend/src/app/containers/ExperimentPage/index.js b/frontend/src/app/containers/ExperimentPage/index.js
@@ -111,7 +111,8 @@ export class ExperimentDetail extends React.Component {
       uninstallExperimentWithSurvey,
       flashMeasurementPanel,
       doShowEolDialog,
-      doShowTourDialog
+      doShowTourDialog,
+      doShowPreFeedbackDialog
     } = this;
 
     const {
@@ -132,7 +133,8 @@ export class ExperimentDetail extends React.Component {
       setScrollY,
       getScrollY,
       sendToGA,
-      addScrollListener
+      addScrollListener,
+      removeScrollListener
     } = this.props;
 
     const {
@@ -148,11 +150,6 @@ export class ExperimentDetail extends React.Component {
       isDisabling
     } = this.state;
 
-    const {
-      title,
-      survey_url
-    } = experiment;
-
     // Loading handled in static with react router; don't return anything if no experiments
     if (experiments.length === 0) {
       return null;
@@ -163,6 +160,11 @@ export class ExperimentDetail extends React.Component {
       return <NotFoundPage {...this.props} />;
     }
 
+    const {
+      title,
+      survey_url
+    } = experiment;
+
     setPageTitleL10N('pageTitleExperiment', experiment);
 
     // Show a 404 page if an experiment is not ready for launch yet
@@ -264,7 +266,9 @@ export class ExperimentDetail extends React.Component {
                 l10nId,
                 sendToGA,
                 doShowEolDialog,
-                addScrollListener
+                doShowPreFeedbackDialog,
+                addScrollListener,
+                removeScrollListener
               }}
             />
             <div id="details">
@@ -421,15 +425,20 @@ export class ExperimentDetail extends React.Component {
     this.setState({ showTourDialog: true });
   };
 
-  uninstallExperimentWithSurvey = evt => {
+  doShowEolDialog = evt => {
     evt.preventDefault();
-    this.uninstallExperiment(evt);
-    this.setState({ showDisableDialog: true });
+    this.setState({ showEolDialog: true });
   };
 
-  doShowEolDialog = evt => {
+  doShowPreFeedbackDialog = evt => {
     evt.preventDefault();
-    this.setState({ showEolDialog: true });
+    this.setState({ showPreFeedbackDialog: true });
+  };
+
+  uninstallExperimentWithSurvey = evt => {
+    evt.preventDefault();
+    this.uninstallExperiment(evt);
+    this.setState({ showDisableDialog: true });
   };
 
   flashMeasurementPanel = () => {

diff --git a/frontend/src/app/containers/ExperimentPage/tests.js b/frontend/src/app/containers/ExperimentPage/tests.js
@@ -17,6 +17,8 @@ import ExperimentDisableDialog from './ExperimentDisableDialog';
 import ExperimentEolDialog from './ExperimentEolDialog';
 import ExperimentTourDialog from './ExperimentTourDialog';
 
+import { PRIVACY_SCROLL_OFFSET } from './DetailsHeader';
+
 describe('app/containers/ExperimentPage', () => {
   const mockExperiment = {
     slug: 'testing',
@@ -119,7 +121,7 @@ describe('app/containers/ExperimentPage:ExperimentDetail', () => {
       setPageTitleL10N: sinon.spy()
     };
 
-    subject = shallow(<ExperimentDetail {...props} />);
+    subject = mount(<ExperimentDetail {...props} />);
   });
 
   const setExperiment = experiment => {
@@ -386,7 +388,6 @@ describe('app/containers/ExperimentPage:ExperimentDetail', () => {
         const mountedSubject = mount(<ExperimentDetail {...mountedProps} />);
 
         expect(props.addScrollListener.called).to.be.true;
-        expect(mountedSubject.state('useStickyHeader')).to.be.false;
         expect(mountedSubject.find('.details-header-wrapper').hasClass('stick')).to.be.false;
 
         const scrollListener = props.addScrollListener.lastCall.args[0];
@@ -395,14 +396,12 @@ describe('app/containers/ExperimentPage:ExperimentDetail', () => {
 
         // HACK: scrollListner() has a setTimeout(..., 1) so let's wait longer.
         setTimeout(() => {
-          expect(mountedSubject.state('useStickyHeader')).to.be.true;
           expect(mountedSubject.find('.details-header-wrapper').hasClass('stick')).to.be.true;
 
           // Now, scroll back.
           scrollY = 10;
           scrollListener();
           setTimeout(() => {
-            expect(mountedSubject.state('useStickyHeader')).to.be.false;
             expect(mountedSubject.find('.details-header-wrapper').hasClass('stick')).to.be.false;
             expect(mountedSubject.find('.details-header-wrapper.stick'))
               .to.have.property('length', 0);
@@ -424,8 +423,7 @@ describe('app/containers/ExperimentPage:ExperimentDetail', () => {
 
         subject.find('.highlight-privacy').simulate('click', mockClickEvent);
         expect(props.setScrollY.lastCall.args[0]).to.equal(
-          elementY + (genericElementHeight - subject.state('privacyScrollOffset')));
-        expect(subject.state('useStickyHeader')).to.be.true;
+          elementY + (genericElementHeight - PRIVACY_SCROLL_OFFSET));
         expect(subject.state('highlightMeasurementPanel')).to.be.true;
 
         // TODO: 5 second delay is too much. Skip until/unless we mock setTimeout.