Skip to content
This repository has been archived by the owner on May 19, 2020. It is now read-only.

Commit

Permalink
Make breadcrumbs component dumb
Browse files Browse the repository at this point in the history
This also fixes the "calling setState on an unmounted component"
warning.

Also, instead of using CSS classes to target DOM nodes in integration
tests, we use `data-test` attributes. We can later strip these out of
the build using a babel plugin, if desired. Note: there are other ways
of doing this, but for the time being the `data-test` attribute best
matches the current code with the least number of changes and is clear
enough. It's also a recommended approach by Kent C Dodds, if that holds
any weight.
  • Loading branch information
jonathaningram committed Oct 10, 2017
1 parent 44c50d2 commit 66f9b93
Show file tree
Hide file tree
Showing 11 changed files with 105 additions and 145 deletions.
6 changes: 3 additions & 3 deletions static_src/components/app_container.jsx
Original file line number Diff line number Diff line change
Expand Up @@ -7,7 +7,7 @@ import { appHealth, worstAppInstanceState } from '../util/health';
import { appStates } from '../constants';
import { config } from 'skin';
import AppStore from '../stores/app_store.js';
import Breadcrumbs from './breadcrumbs.jsx';
import Breadcrumbs from './breadcrumbs';
import DomainStore from '../stores/domain_store.js';
import EnvStore from '../stores/env_store.js';
import RouteStore from '../stores/route_store.js';
Expand Down Expand Up @@ -193,7 +193,7 @@ export default class AppContainer extends React.Component {


render() {
const { app, envRequest, envUpdateError } = this.state;
const { org, space, app, envRequest, envUpdateError } = this.state;

let loading = <Loading text="Loading app" />;
let content = <div>{ loading }</div>;
Expand All @@ -211,7 +211,7 @@ export default class AppContainer extends React.Component {
<div>
<div className={ this.styler('grid') }>
<div className={ this.styler('grid-width-12') }>
<Breadcrumbs />
<Breadcrumbs org={org} space={space} app={app} />
<PageHeader title={ title }>
{ this.error }
{ this.openApp }
Expand Down
79 changes: 0 additions & 79 deletions static_src/components/breadcrumbs.jsx

This file was deleted.

51 changes: 51 additions & 0 deletions static_src/components/breadcrumbs/breadcrumbs.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,51 @@
import React from 'react';

import { appPropType } from '../../stores/app_store';
import { orgPropType } from '../../stores/org_store';
import { spacePropType } from '../../stores/space_store';
import Item from './breadcrumbs_item';
import { orgHref, spaceHref } from '../../util/url';

const propTypes = {
org: orgPropType.isRequired,
space: spacePropType,
app: appPropType
};

const Breadcrumbs = ({ org, space, app }) => {
const items = [
<Item key="home" href="/" data-test="overview">
Overview
</Item>
];

if (org && space) {
items.push(
<Item key={org.guid} href={orgHref(org)} data-test="org">
{org.name}
</Item>
);
}

if (org && space && app) {
items.push(
<Item
key={space.guid}
href={spaceHref(org, space)}
data-test="space"
>
{space.name}
</Item>
);
}

return (
<ol className="breadcrumbs" data-test="breadcrumbs">
{items}
</ol>
);
};

Breadcrumbs.propTypes = propTypes;

export default Breadcrumbs;
23 changes: 23 additions & 0 deletions static_src/components/breadcrumbs/breadcrumbs_item.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,23 @@
import React from 'react';
import PropTypes from 'prop-types';

const propTypes = {
children: PropTypes.node.isRequired,
href: PropTypes.string
};

const BreadcrumbsItem = ({ children, href }) => (
<li className="breadcrumbs-item">
{href ? (
<a className="breadcrumbs-item-link" href={href}>
{children}
</a>
) : (
<span className="breadcrumbs-item-current">{children}</span>
)}
</li>
);

BreadcrumbsItem.propTypes = propTypes;

export default BreadcrumbsItem;
1 change: 1 addition & 0 deletions static_src/components/breadcrumbs/index.js
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
export { default } from './breadcrumbs';
37 changes: 0 additions & 37 deletions static_src/components/breadcrumbs_item.jsx

This file was deleted.

9 changes: 5 additions & 4 deletions static_src/components/org_container.jsx
Original file line number Diff line number Diff line change
Expand Up @@ -7,7 +7,7 @@ import style from 'cloudgov-style/css/cloudgov-style.css';


import AppCountStatus from './app_count_status.jsx';
import Breadcrumbs from './breadcrumbs.jsx';
import Breadcrumbs from './breadcrumbs';
import EntityIcon from './entity_icon.jsx';
import EntityEmpty from './entity_empty.jsx';
import Loading from './loading.jsx';
Expand Down Expand Up @@ -126,12 +126,13 @@ export default class OrgContainer extends React.Component {
}

render() {
const { org } = this.state;
const state = this.state;
let loading = <Loading text="Loading organization" />;
let content = <div>{ loading }</div>;
const title = (
<span>
<EntityIcon entity="org" iconSize="large" /> { state.org.name }
<EntityIcon entity="org" iconSize="large" /> { org.name }
</span>
);
const spaces = !state.spaces.length ? this.emptyState :
Expand All @@ -147,7 +148,7 @@ export default class OrgContainer extends React.Component {

if (state.empty) {
content = <h4 className="test-none_message">No organizations</h4>;
} else if (!state.loading && state.org) {
} else if (!state.loading && org) {
const allApps = this.allApps();
const allServices = this.allServices();

Expand All @@ -156,7 +157,7 @@ export default class OrgContainer extends React.Component {
<div className={ this.styler('grid') }>
<div className={ this.styler('grid') }>
<div className={ this.styler('grid-width-12') }>
<Breadcrumbs />
<Breadcrumbs org={org} />
<PageHeader title={ title } />
</div>
</div>
Expand Down
11 changes: 6 additions & 5 deletions static_src/components/space_container.jsx
Original file line number Diff line number Diff line change
Expand Up @@ -4,7 +4,7 @@ import React from 'react';

import AppCountStatus from './app_count_status.jsx';
import AppList from '../components/app_list.jsx';
import Breadcrumbs from './breadcrumbs.jsx';
import Breadcrumbs from './breadcrumbs';
import EntityIcon from './entity_icon.jsx';
import Loading from './loading.jsx';
import Marketplace from './marketplace.jsx';
Expand Down Expand Up @@ -77,20 +77,21 @@ export default class SpaceContainer extends React.Component {
return <Loading />;
}

const { currentOrg: org, space } = this.state;

let main = <div></div>;
const title = (
<span>
<EntityIcon entity="space" iconSize="large" /> { this.state.space.name }
<EntityIcon entity="space" iconSize="large" /> { space.name }
</span>
);

if (this.state.space && this.state.space.guid) {
const space = this.state.space;
if (space && space.guid) {
main = (
<div>
<div className={ this.styler('grid') }>
<div className={ this.styler('grid-width-12') }>
<Breadcrumbs />
<Breadcrumbs org={org} space={space} />
<PageHeader title={ title } />
</div>
</div>
Expand Down
11 changes: 6 additions & 5 deletions static_src/stores/org_store.js
Original file line number Diff line number Diff line change
@@ -1,15 +1,16 @@

/*
* Store for org data. Will store and update org data on changes from UI and
* server.
*/
import PropTypes from 'prop-types';

import AppDispatcher from '../dispatcher';
import BaseStore from './base_store.js';
import LoginStore from './login_store.js';
import { orgActionTypes } from '../constants.js';
import Quicklook from '../models/quicklook';

export const orgPropType = PropTypes.shape({
guid: PropTypes.string.isRequired,
name: PropTypes.string.isRequired
});

export class OrgStore extends BaseStore {
constructor() {
super();
Expand Down
6 changes: 6 additions & 0 deletions static_src/stores/space_store.js
Original file line number Diff line number Diff line change
Expand Up @@ -4,10 +4,16 @@
*/

import Immutable from 'immutable';
import PropTypes from 'prop-types';

import BaseStore from './base_store.js';
import { orgActionTypes, spaceActionTypes } from '../constants.js';

export const spacePropType = PropTypes.shape({
guid: PropTypes.string.isRequired,
name: PropTypes.string.isRequired
});

class SpaceStore extends BaseStore {
constructor() {
super();
Expand Down
16 changes: 4 additions & 12 deletions static_src/test/functional/pageobjects/breadcrumbs.element.js
Original file line number Diff line number Diff line change
@@ -1,19 +1,12 @@

import BaseElement from './base.element';

// https://www.martinfowler.com/bliki/PageObject.html
//
// Represents a DOM element for making assertions against. This makes it
// easier to abstract some of the webdriver details from the UI component.

// TODO attach to class as static property
const breadcrumbs = '.test-breadcrumbs';
const breadcrumbs = '[data-test="breadcrumbs"]';

const selectors = {
primary: breadcrumbs,
overview: `${breadcrumbs} li:first-child a`,
org: `${breadcrumbs} li:nth-child(1) a`,
space: `${breadcrumbs} li:last-child a`
overview: `${breadcrumbs} [data-test="overview"]`,
org: `${breadcrumbs} [data-test="org"]`,
space: `${breadcrumbs} [data-test="space"]`
};

export default class Breadcrumbs extends BaseElement {
Expand All @@ -40,4 +33,3 @@ export default class Breadcrumbs extends BaseElement {
}

Breadcrumbs.primarySelector = selectors.primary;

0 comments on commit 66f9b93

Please sign in to comment.