Skip to content
This repository has been archived by the owner on Jan 17, 2023. It is now read-only.

Fix #2694 Link to the graduation reports in the blog. #2875

Merged
merged 14 commits into from
Oct 2, 2017

Conversation

fzzzy
Copy link
Contributor

@fzzzy fzzzy commented Sep 20, 2017

No description provided.

@@ -123,7 +123,7 @@ export default class LocalizedHtml extends Localized {
}
);
}
throw new Error('ftl string did not have as many anchors as the jsx');
throw new Error(`ftl string "${this.props.id}" did not have as many anchors as the jsx`);
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is unrelated, but it helped me debug something while working on this branch, might as well get it into master.

@fzzzy
Copy link
Contributor Author

fzzzy commented Sep 20, 2017

Still need to:

And fix conflicts

# Conflicts:
#	frontend/test-setup.js
#	gulpfile.babel.js
#	package.json
@codecov-io
Copy link

codecov-io commented Sep 25, 2017

Codecov Report

Merging #2875 into master will increase coverage by 0.06%.
The diff coverage is 93.75%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #2875      +/-   ##
==========================================
+ Coverage   94.78%   94.85%   +0.06%     
==========================================
  Files          88       90       +2     
  Lines        2243     2254      +11     
==========================================
+ Hits         2126     2138      +12     
+ Misses        117      116       -1
Impacted Files Coverage Δ
...rontend/test/app/containers/ExperimentPage-test.js 99.63% <ø> (-0.01%) ⬇️
frontend/src/app/components/LocalizedHtml.js 31.42% <0%> (ø) ⬆️
...ontend/src/app/components/GraduatedNotice/tests.js 100% <100%> (ø)
frontend/src/app/containers/ExperimentPage.js 84.5% <100%> (+0.39%) ⬆️
...ontend/src/app/components/GraduatedNotice/index.js 100% <100%> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 9796496...f6de18b. Read the comment docs.

@fzzzy fzzzy requested a review from flodolo as a code owner September 25, 2017 20:57
@fzzzy
Copy link
Contributor Author

fzzzy commented Sep 25, 2017

The test failures seem to be a temporary problem with circle.

@fzzzy fzzzy changed the title WIP Fix #2694 Link to the graduation reports in the blog. Fix #2694 Link to the graduation reports in the blog. Sep 25, 2017
@fzzzy
Copy link
Contributor Author

fzzzy commented Sep 25, 2017

The temporary circle problem obscured the lint errors.

[[graduated]]
graduationReportButton=Graduation Report
graduationNoticeTitle=This experiment has ended
graduationNoticeReportNotReady=We are working on a full report. Check back soon for the details.
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why are we duplicating two existing strings? Are those used anywhere?

experimentGradReportPendingTitle = This experiment has ended
experimentGradReportPendingCopy = We are working on a full report. Check back soon for the details.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you for pointing this out! I searched for "graduation" but there were no hits, and "grad" hit a bunch of other things that were not related. I will re-use the existing strings.




.graduated-notice {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit, extra new lines

@@ -0,0 +1,19 @@


import React from 'react';
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit, new lines at beginning of file


import { storiesOf } from '@storybook/react';

import GraduatedNotice from './index';
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

inconsistent spacing of new lines

@fzzzy
Copy link
Contributor Author

fzzzy commented Sep 26, 2017

Since this has a string change, this shouldn't land until the new sprint starts.

@fzzzy fzzzy added this to the Sprint 33 milestone Sep 27, 2017
@lmorchard lmorchard merged commit af1db23 into mozilla:master Oct 2, 2017
@lmorchard
Copy link
Contributor

New sprint is go, and we updated staging. Merging!

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants