From 9a3a0a164333b3e55e7a1d3b3a6182fbf47bf70f Mon Sep 17 00:00:00 2001 From: Jonathan Ingram Date: Tue, 10 Oct 2017 17:34:50 +1100 Subject: [PATCH 1/3] Make breadcrumbs component dumb 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. --- static_src/components/app_container.jsx | 7 +- static_src/components/breadcrumbs.jsx | 75 ------------------- .../components/breadcrumbs/breadcrumbs.js | 47 ++++++++++++ .../breadcrumbs/breadcrumbs_item.js | 26 +++++++ static_src/components/breadcrumbs/index.js | 1 + static_src/components/breadcrumbs_item.jsx | 29 ------- static_src/components/org_container.jsx | 10 +-- static_src/components/space_container.jsx | 12 +-- static_src/stores/org_store.js | 11 +-- static_src/stores/space_store.js | 6 ++ .../pageobjects/breadcrumbs.element.js | 16 +--- 11 files changed, 104 insertions(+), 136 deletions(-) delete mode 100644 static_src/components/breadcrumbs.jsx create mode 100644 static_src/components/breadcrumbs/breadcrumbs.js create mode 100644 static_src/components/breadcrumbs/breadcrumbs_item.js create mode 100644 static_src/components/breadcrumbs/index.js delete mode 100644 static_src/components/breadcrumbs_item.jsx diff --git a/static_src/components/app_container.jsx b/static_src/components/app_container.jsx index 93f3579b..48aaf4bc 100644 --- a/static_src/components/app_container.jsx +++ b/static_src/components/app_container.jsx @@ -1,4 +1,3 @@ - import React from 'react'; import Action from './action.jsx'; @@ -7,7 +6,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'; @@ -190,7 +189,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 = ; let content =
{ loading }
; @@ -208,7 +207,7 @@ export default class AppContainer extends React.Component {
- + { this.error } { this.openApp } diff --git a/static_src/components/breadcrumbs.jsx b/static_src/components/breadcrumbs.jsx deleted file mode 100644 index 1c4fa971..00000000 --- a/static_src/components/breadcrumbs.jsx +++ /dev/null @@ -1,75 +0,0 @@ -import React from 'react'; -import AppStore from '../stores/app_store'; -import BreadcrumbsItem from './breadcrumbs_item.jsx'; -import OrgStore from '../stores/org_store'; -import SpaceStore from '../stores/space_store'; -import { orgHref, spaceHref } from '../util/url'; - -function stateSetter() { - const app = AppStore.get(AppStore.currentAppGuid); - const space = SpaceStore.get(SpaceStore.currentSpaceGuid); - const org = OrgStore.get(OrgStore.currentOrgGuid); - - return { - app, - org, - space - }; -} - -export default class Breadcrumbs extends React.Component { - constructor(props) { - super(props); - - this.state = stateSetter(); - this._onChange = this._onChange.bind(this); - } - - componentDidMount() { - AppStore.addChangeListener(this._onChange); - OrgStore.addChangeListener(this._onChange); - SpaceStore.addChangeListener(this._onChange); - } - - componentWillUnmount() { - AppStore.removeChangeListener(this._onChange); - OrgStore.removeChangeListener(this._onChange); - SpaceStore.removeChangeListener(this._onChange); - } - - _onChange() { - this.setState(stateSetter()); - } - - render() { - let breadcrumbs = []; - const { org, space, app } = this.state; - - // Add the parent states - if (org && org.name) { - breadcrumbs.push(Overview); - } - - if (org && space && space.name) { - breadcrumbs.push( - { org.name } - ); - } - - if (org && space && app && app.name) { - breadcrumbs.push( - ( - - { space.name } - - ) - ); - } - - return ( -
    - { breadcrumbs } -
- ); - } -} diff --git a/static_src/components/breadcrumbs/breadcrumbs.js b/static_src/components/breadcrumbs/breadcrumbs.js new file mode 100644 index 00000000..bf4833d7 --- /dev/null +++ b/static_src/components/breadcrumbs/breadcrumbs.js @@ -0,0 +1,47 @@ +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 = [ + + Overview + + ]; + + if (org && space) { + items.push( + + {org.name} + + ); + } + + if (org && space && app) { + items.push( + + {space.name} + + ); + } + + return ( +
    + {items} +
+ ); +}; + +Breadcrumbs.propTypes = propTypes; + +export default Breadcrumbs; diff --git a/static_src/components/breadcrumbs/breadcrumbs_item.js b/static_src/components/breadcrumbs/breadcrumbs_item.js new file mode 100644 index 00000000..1739f146 --- /dev/null +++ b/static_src/components/breadcrumbs/breadcrumbs_item.js @@ -0,0 +1,26 @@ +import React from 'react'; +import PropTypes from 'prop-types'; + +const propTypes = { + children: PropTypes.node.isRequired, + href: PropTypes.string, + testLabel: PropTypes.string +}; + +const BreadcrumbsItem = ({ children, href, testLabel }) => ( +
  • + {href ? ( + + {children} + + ) : ( + + {children} + + )} +
    • +); + +BreadcrumbsItem.propTypes = propTypes; + +export default BreadcrumbsItem; diff --git a/static_src/components/breadcrumbs/index.js b/static_src/components/breadcrumbs/index.js new file mode 100644 index 00000000..7b9b184b --- /dev/null +++ b/static_src/components/breadcrumbs/index.js @@ -0,0 +1 @@ +export { default } from './breadcrumbs'; diff --git a/static_src/components/breadcrumbs_item.jsx b/static_src/components/breadcrumbs_item.jsx deleted file mode 100644 index b2269349..00000000 --- a/static_src/components/breadcrumbs_item.jsx +++ /dev/null @@ -1,29 +0,0 @@ - -import PropTypes from 'prop-types'; -import React from 'react'; - -const propTypes = { - children: PropTypes.node.isRequired, - url: PropTypes.string -}; - -export default class BreadcrumbsItem extends React.Component { - render() { - const url = this.props.url; - const content = url ? - ( - - { this.props.children } - - ) - : { this.props.children }; - - return ( -
  • - { content } -
    • - ); - } -} - -BreadcrumbsItem.propTypes = propTypes; diff --git a/static_src/components/org_container.jsx b/static_src/components/org_container.jsx index db811520..b9a5b0e6 100644 --- a/static_src/components/org_container.jsx +++ b/static_src/components/org_container.jsx @@ -1,9 +1,8 @@ - import React from 'react'; import { config } from 'skin'; 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'; @@ -121,12 +120,13 @@ export default class OrgContainer extends React.Component { } render() { + const { org } = this.state; const state = this.state; let loading = ; let content =
    { loading }
    ; const title = ( - { state.org.name } + { org.name } ); const spaces = !state.spaces.length ? this.emptyState : @@ -142,7 +142,7 @@ export default class OrgContainer extends React.Component { if (state.empty) { content =

    No organizations

    ; - } else if (!state.loading && state.org) { + } else if (!state.loading && org) { const allApps = this.allApps(); const allServices = this.allServices(); @@ -151,7 +151,7 @@ export default class OrgContainer extends React.Component {
    - +
    diff --git a/static_src/components/space_container.jsx b/static_src/components/space_container.jsx index 6aec665a..e0f59f4b 100644 --- a/static_src/components/space_container.jsx +++ b/static_src/components/space_container.jsx @@ -1,10 +1,9 @@ - import PropTypes from 'prop-types'; 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'; @@ -74,20 +73,21 @@ export default class SpaceContainer extends React.Component { return ; } + const { currentOrg: org, space } = this.state; + let main =
    ; const title = ( - { this.state.space.name } + { space.name } ); - if (this.state.space && this.state.space.guid) { - const space = this.state.space; + if (space && space.guid) { main = (
    - +
    diff --git a/static_src/stores/org_store.js b/static_src/stores/org_store.js index 25086387..15d24307 100644 --- a/static_src/stores/org_store.js +++ b/static_src/stores/org_store.js @@ -1,8 +1,4 @@ - -/* - * 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'; @@ -10,6 +6,11 @@ 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(); diff --git a/static_src/stores/space_store.js b/static_src/stores/space_store.js index 74fe60a8..592f405d 100644 --- a/static_src/stores/space_store.js +++ b/static_src/stores/space_store.js @@ -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(); diff --git a/static_src/test/functional/pageobjects/breadcrumbs.element.js b/static_src/test/functional/pageobjects/breadcrumbs.element.js index 42bce331..257f3411 100644 --- a/static_src/test/functional/pageobjects/breadcrumbs.element.js +++ b/static_src/test/functional/pageobjects/breadcrumbs.element.js @@ -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 { @@ -40,4 +33,3 @@ export default class Breadcrumbs extends BaseElement { } Breadcrumbs.primarySelector = selectors.primary; - From 13dd025925680e28522122e3197de721e99f4f14 Mon Sep 17 00:00:00 2001 From: Jonathan Ingram Date: Fri, 13 Oct 2017 09:27:57 +1100 Subject: [PATCH 2/3] Use BreadcrumbsItem instead of Item for import --- .../components/breadcrumbs/breadcrumbs.js | 18 +++++++++++------- 1 file changed, 11 insertions(+), 7 deletions(-) diff --git a/static_src/components/breadcrumbs/breadcrumbs.js b/static_src/components/breadcrumbs/breadcrumbs.js index bf4833d7..238c1c11 100644 --- a/static_src/components/breadcrumbs/breadcrumbs.js +++ b/static_src/components/breadcrumbs/breadcrumbs.js @@ -3,7 +3,7 @@ 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 BreadcrumbsItem from './breadcrumbs_item'; import { orgHref, spaceHref } from '../../util/url'; const propTypes = { @@ -14,24 +14,28 @@ const propTypes = { const Breadcrumbs = ({ org, space, app }) => { const items = [ - + Overview - + ]; if (org && space) { items.push( - + {org.name} - + ); } if (org && space && app) { items.push( - + {space.name} - + ); } From 7d9f566caf7f056c5443469e804d14137b4ae9b4 Mon Sep 17 00:00:00 2001 From: Jonathan Ingram Date: Fri, 13 Oct 2017 09:28:50 +1100 Subject: [PATCH 3/3] Add comment back into org_store.js --- static_src/stores/org_store.js | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/static_src/stores/org_store.js b/static_src/stores/org_store.js index 15d24307..815ed3ea 100644 --- a/static_src/stores/org_store.js +++ b/static_src/stores/org_store.js @@ -1,3 +1,7 @@ +/* + * 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';