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

Commit

Permalink
Initial work toward new component bundle directories (#2859)
Browse files Browse the repository at this point in the history
* Tweaks to support component directory bundles

- Storybook now also finds stories in frontend/src/app/**/stories.js

- `npm run test` now also finds tests in frontend/src/app/**/tests.js

- New /static/app/app.js.css stylesheet built from Sass styles imported
  by components

- Hacks to ignore .scss imports when using component modules outside
  Webpack for static page build & tests

- Update flowconfig to ignore .scss imports

- Small eslint upgrade so CLI matches gulp-eslint

* Reorganize ExperimentRowCard component into bundle directory

* Reorganize UpdateList component into bundle directory

* Begin to extract IncompatibleAddons component from ExperimentPage container

* Rename stories.jsx -> stories.js; See also: airbnb/javascript#985 (comment)

* Quick & dirty revision to Storybook docs to incorporate new component bundle proposal

Issue #2807
  • Loading branch information
lmorchard authored Sep 22, 2017
1 parent aa339c9 commit f4000e2
Show file tree
Hide file tree
Showing 22 changed files with 376 additions and 107 deletions.
8 changes: 7 additions & 1 deletion .eslintrc
Original file line number Diff line number Diff line change
Expand Up @@ -34,6 +34,8 @@ rules:
comma-dangle: [2, never]
indent: [2, 2, {SwitchCase: 1}]
max-len: [0]
one-var: off
one-var-declaration-per-line: off
no-console: 1
no-multi-spaces: [0]
no-param-reassign: [0]
Expand All @@ -44,4 +46,8 @@ rules:
linebreak-style: [0]
"import/no-extraneous-dependencies":
- error
- { devDependencies: [ "./frontend/{stories,tasks,tests}/**/*.{js,jsx}" ] }
-
devDependencies:
- '**/tests.js'
- '**/stories.{js,jsx}'
- "./frontend/{stories,tasks,tests}/**/*.{js,jsx}"
2 changes: 2 additions & 0 deletions .storybook/config.js
Original file line number Diff line number Diff line change
Expand Up @@ -6,9 +6,11 @@ import '../frontend/build/static/styles/experiments.css';
import '../frontend/build/static/styles/main.css';

const req = require.context('../frontend/stories', true, /\-story\.jsx?$/);
const reqInSrcTree = require.context('../frontend/src/app', true, /\/stories.jsx?$/);

function loadStories() {
req.keys().forEach((filename) => req(filename));
reqInSrcTree.keys().forEach((filename) => reqInSrcTree(filename));
}

configure(loadStories, module);
23 changes: 23 additions & 0 deletions .storybook/webpack.config.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,23 @@
const path = require('path');

// Export a function. Accept the base config as the only param.
module.exports = (storybookBaseConfig, configType) => {
// configType has a value of 'DEVELOPMENT' or 'PRODUCTION'
// You can change the configuration based on that.
// 'PRODUCTION' is used when building the static version of storybook.

// Make whatever fine-grained changes you need
storybookBaseConfig.module.rules.push({
test: /\.s?css$/,
loaders: ["style-loader", "css-loader", "sass-loader"],
include: path.resolve(__dirname, '../')
});

storybookBaseConfig.module.rules.push({
test: /\.(png|jpg|gif|svg)$/,
loaders: ['url-loader']
});

// Return the altered config
return storybookBaseConfig;
};
159 changes: 139 additions & 20 deletions docs/development/storybook.md
Original file line number Diff line number Diff line change
Expand Up @@ -26,26 +26,6 @@ All of these are conditions that are difficult or tedious to reproduce, so
Storybook offers us a way to preview how components work without having to do
the manual steps.

## Stories

[The Storybook site has good documentation on writing stories][storydocs],
so we won't reproduce them here.

Stories for Test Pilot are located at [`frontend/stories`][storydir]. The
directory structure mostly mirrors what's found under `frontend/src`. For
example, here's the path to the news updates feed and its associated
collection of stories:

* [`frontend/src/app/components/UpdateList.js`](../frontend/src/app/components/UpdateList.js)
* [`frontend/stories/app/components/UpdateList-story.jsx`](../frontend/src/app/components/UpdateList.js)

When making changes to a component, you should ensure that:
1. a module of stories for that component exists; and
1. that the module contains stories capturing the full range of your component's behavior.

Storybook is a pretty new technology for us at the time of this writing, so
it's likely that new work will need to backfill both of the above.

## Local development

If you've got a local development environment working [per the quickstart
Expand Down Expand Up @@ -97,3 +77,142 @@ Where `{git-commit-id}` is the hash of the latest commit to the branch.
[storydir]: ../../frontend/stories/
[quickstart]: quickstart.md
[repo]: https://github.com/mozilla/testpilot/

## Definitions

- **Component** - an independent, reusable piece of UI. May contain additional components.
- **Story** - A variation of a component’s state, such as a hover, loading, or error state.
- **Container** - a component that is [`connect`](https://github.com/reactjs/react-redux/blob/master/docs/api.md#connectmapstatetoprops-mapdispatchtoprops-mergeprops-options)ed to the Redux store.
- **Dependent components** - components that are only used in the context of a parent component. For example, a `ListItem` component may only be used by a `List` component.

## Changes to code

In order to idiomatically accommodate a shift to use Storybook, substantial pieces of the Test Pilot site will be refactored. The following changes will be made:

- Components will be refactored to be more self-containing (see Appendix 1).
- Dependent components will be defined in the same file as the parent component.
- The parent component should be the default export for the module, but dependent components should also be exported.
- CSS will be tightly coupled with components.
- Each component will have its own CSS file that is imported into the component source.
- Each component’s styles will be namespaced with a single CSS class, which will be used as a prefix for any children classes.
- Any reuse of styles will be implemented as SCSS mix-ins, rather than with utility CSS classes.
- Stories will be tightly coupled with components.
- Each component will have its own `stories.js` file in the component directory.
- That file will contain each story for the component itself, as well as any dependent components.
- Connected versions of container components will be their default export.
- Unconnected versions will also be exported, for use in stories.
- There will be a single top-level `<App>` component that serves as the entry point to the application. It will contain any static components (such as the header and footer), and exactly one store-connected component for the current view.
- All connected components will live in `src/containers`.
- Each container will have its own `mapStateToProps` and `mapDispatchToProps` that provide the bare minimum state and actions required for the view to function, eliminate any potentially-expensive changes to the DOM from irrelevant changes to the store.
- All other components will live in `src/components`.

## Changes to process

This change will necessitate changes to both the deployment process and in how UX and engineering interact.

### Deployment and release

In addition to [other, unrelated changes](https://public.etherpad-mozilla.org/p/testpilot-new-branch-process) to our build and deployment processes, Storybook will be deployed for most commits to the `mozilla/testpilot` repository.

It will work like this:

1. For each commit on any branch of `mozilla/testpilot`, CircleCI will build the site.
2. If the tests pass, CI will deploy a static copy of the Storybook for that commit to a special S3 bucket.
3. If there is an associated pull request for the commit, a bot will comment on the pull requests with links to both the Storybook for the commit and for the latest commit for the pull request.

A dashboard will be built with links to the Storybook for each pull request and commit. Deployed Storybooks will be pruned every 3 months.

### UX and engineering

With the UX and engineering teams on opposite sides of the globe, a process to work on UX changes or new features is designed to minimize back-and-forth between teams. It works like this:

1. UX proposes new designs and delivers them will full-page mocks highlighting any implied changes.
- This includes any variations at smaller breakpoints (especially for containers).
- This includes a set of suggested states: hovers, errors, loading, etc. These do not need to be visually specified at this stage, just indicated as desired.
2. Engineering reviews the mocks, and delivers back to UX:
- A list of all existing components and stories that are changed by the mocks.
- A list of any new components implied by the mockups.
- A list of all the stories to be defined for each new component, including the ones suggested
3. UX provides redline-level specifications for each component and story that is either new or affected by the proposed change, in isolation.
- This will allow UX to create and maintain an accurate component of source files for each component.
- Each component and story should be clearly named.
4. Engineering builds the proposed components and stories and opens a pull request with them.
- The names of each component (i.e. `storiesOf(‘a component name’)` and story (`.add(‘story name’)` should match the ones provided in the UX source material for easy cross-referencing.
- The UX contact should be marked as a reviewer for the pull request.
5. UX reviews the storybook for the pull requests, requesting any appropriate changes before signing off on and merging the new feature.

## Appendix 1: Example component structure.

```
src
└ containers
| └ homepage
| | └ index.js
| | └ index.scss
| | └ stories.js
| | └ tests.js
└ components
| └ install-button
| | └ index.js
| | └ index.scss
| | └ stories.js
| | └ tests.js
└ util.scss
```

### `components/install-button/index.js`

``` javascript
import './index.scss';

export default class InstallButton extends Component {
renderIcon() {
const { icon } = this.props;
if (icon) {
return (
<span className={`install-button--icon install-button--icon--${icon}`} />
);
}
return null;
}

render() {
const { text } = this.props;
return (
<button className="install-button">{this.renderIcon()}{text}</button>
);
}
}
```

### `components/install-button/index.scss`

``` css
@import '../../util.scss';

.install-button {
@include button();

background: #000;
color: #FFF;

.install-button--icon {
display: inline-block;
margin-right: 4px;
}
}
```

### `containers/homepage/index.js`

``` javascript
export class Homepage extends Component {
render() {
return <p>Hi!</p>
}
}

export default connect(Homepage)(mapStateToProps, mapDispatchToProps);
```


5 changes: 5 additions & 0 deletions frontend/.flowconfig
Original file line number Diff line number Diff line change
Expand Up @@ -11,4 +11,9 @@
[libs]
../flow-typed

[options]
module.file_ext=.scss
module.file_ext=.js
module.file_ext=.jsx

module.name_mapper='.*\.scss$' -> 'empty/object'
Original file line number Diff line number Diff line change
Expand Up @@ -4,11 +4,11 @@ import classnames from 'classnames';
import { Localized } from 'fluent-react/compat';
import React from 'react';

import { buildSurveyURL, experimentL10nId } from '../lib/utils';
import { buildSurveyURL, experimentL10nId } from '../../lib/utils';

import type { InstalledExperiments } from '../reducers/addon';
import type { InstalledExperiments } from '../../reducers/addon';

import ExperimentPlatforms from './ExperimentPlatforms';
import ExperimentPlatforms from '../ExperimentPlatforms';

const ONE_DAY = 24 * 60 * 60 * 1000;
const ONE_WEEK = 7 * ONE_DAY;
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -4,8 +4,8 @@ import { storiesOf } from '@storybook/react';
import { action } from '@storybook/addon-actions';
import { withKnobs, object, boolean } from '@storybook/addon-knobs';

import ExperimentRowCard from '../../../src/app/components/ExperimentRowCard';
import LayoutWrapper from '../../../src/app/components/LayoutWrapper';
import ExperimentRowCard from './index';
import LayoutWrapper from '../LayoutWrapper';

const ONE_DAY = 24 * 60 * 60 * 1000;
const ONE_WEEK = 7 * ONE_DAY;
Expand Down
Original file line number Diff line number Diff line change
@@ -1,11 +1,12 @@
/* global describe, beforeEach, it */
import React from 'react';
import { expect } from 'chai';
import sinon from 'sinon';
import { shallow } from 'enzyme';
import { findLocalizedById } from '../util';
import moment from 'moment';
import { findLocalizedById } from '../../../../test/app/util';

import ExperimentRowCard from '../../../src/app/components/ExperimentRowCard';
import ExperimentRowCard from './index';

describe('app/components/ExperimentRowCard', () => {
let mockExperiment, mockClickEvent, props, subject;
Expand Down Expand Up @@ -112,7 +113,7 @@ describe('app/components/ExperimentRowCard', () => {

subject.setProps({
enabled: false,
getExperimentLastSeen: sinon.spy(experiment => moment().valueOf())
getExperimentLastSeen: sinon.spy(() => moment().valueOf())
});

expect(props.getExperimentLastSeen.called).to.be.true;
Expand All @@ -127,7 +128,7 @@ describe('app/components/ExperimentRowCard', () => {
props = { ...props,
enabled: false,
getExperimentLastSeen:
sinon.spy(experiment => moment().subtract(1.5, 'week').valueOf()),
sinon.spy(() => moment().subtract(1.5, 'week').valueOf()),
experiment: { ...mockExperiment,
modified: moment().subtract(1, 'week').utc()
}
Expand All @@ -145,7 +146,7 @@ describe('app/components/ExperimentRowCard', () => {

subject.setProps({
enabled: false,
getExperimentLastSeen: sinon.spy(experiment => moment().valueOf())
getExperimentLastSeen: sinon.spy(() => moment().valueOf())
});

expect(props.getExperimentLastSeen.called).to.be.true;
Expand All @@ -157,15 +158,15 @@ describe('app/components/ExperimentRowCard', () => {
expect(findLocalizedById(subject, 'experimentListEndingTomorrow')).to.have.property('length', 0);
subject.setProps({ experiment: { ...mockExperiment,
completed: moment().add(23, 'hours').utc()
}});
} });
expect(findLocalizedById(subject, 'experimentListEndingTomorrow')).to.have.property('length', 1);
});

it('should show a "soon" status message when ending in one week', () => {
expect(findLocalizedById(subject, 'experimentListEndingSoon')).to.have.property('length', 0);
subject.setProps({ experiment: { ...mockExperiment,
completed: moment().add(6, 'days').utc()
}});
} });
expect(findLocalizedById(subject, 'experimentListEndingSoon')).to.have.property('length', 1);
});

Expand Down Expand Up @@ -198,7 +199,7 @@ describe('app/components/ExperimentRowCard', () => {
hasAddon: true
});
expect(findLocalizedById(subject, 'experimentCardManage')).to.have.property('length', 1);
})
});

it('should ping GA when manage is clicked', () => {
subject.find('.experiment-summary').simulate('click', mockClickEvent);
Expand Down
59 changes: 59 additions & 0 deletions frontend/src/app/components/IncompatibleAddons/index.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,59 @@
// @flow

import React from 'react';
import { Localized } from 'fluent-react/compat';
import LocalizedHtml from '../LocalizedHtml';

import './index.scss';

type IncompatibleAddonsProps = {
experiment: Object,
installedAddons: Array<string>
};

export default class IncompatibleAddons extends React.Component {
props: IncompatibleAddonsProps

render() {
const { incompatible } = this.props.experiment;
const installed = this.getIncompatibleInstalled(incompatible);
if (installed.length === 0) return null;

const helpUrl = 'https://support.mozilla.org/kb/disable-or-remove-add-ons';

return (
<section className="incompatible-addons">
<header>
<Localized id="incompatibleHeader">
<h3>
This experiment may not be compatible with add-ons you have installed.
</h3>
</Localized>
<LocalizedHtml id="incompatibleSubheader">
<p>
We recommend <a href={helpUrl}>disabling these add-ons</a> before activating this experiment:
</p>
</LocalizedHtml>
</header>
<main>
<ul>
{installed.map(guid => (
<li key={guid}>{incompatible[guid]}</li>
))}
</ul>
</main>
</section>
);
}

getIncompatibleInstalled(incompatible: Object) {
if (!incompatible) {
return [];
}
const installed = this.props.installedAddons || [];
return Object.keys(incompatible).filter(guid => (
installed.indexOf(guid) !== -1
));
}

}
2 changes: 2 additions & 0 deletions frontend/src/app/components/IncompatibleAddons/index.scss
Original file line number Diff line number Diff line change
@@ -0,0 +1,2 @@
// TODO: migrate <IncompatibleAddons> styles from frontend/src/styles/components/_Warning.scss

Loading

0 comments on commit f4000e2

Please sign in to comment.