[UI Framework] Migrate from a handrolled interface for building examples to using React Components directly.

[cjcenizal]

Per advice from @kjbekkelund

[kimjoar]

👍 I like this approach much better.

[kimjoar]

This is very "un-ES6-y". This should rather be:

export openCodeViewer: source => ({
  // ...
})

export closeCodeViewer: () => ({
  // ...
})

Then you can import { openCodeViewer } from '...' instead. This also makes tree shaking and other things possible (not necessarily relevant for this use case, I just think an ES6 module should almost never default export an object that just contains functions either way).

[kimjoar]

Have you double-check that you don't get a new ref every time here? Should probably just use React.PureComponent instead, which does a shallow compare on props and state.

[cjcenizal]

Good call, it is always a different reference.

[kimjoar]

registerSection shouldn't be on context, rather rely on connect to inject these into the component instead. I would create a GuideSection container that does it.

(context is an escape hatch, so prefer not to use it. In this case you'd still actually depend on context, but through react-redux instead. I think that's okey, as it's a "stricter api")

[kimjoar]

On that context note: all of these should be removed from context and rather fetched from the redux store in containers where they are needed (so either pass them through as props all the way down, or insert containers in the tree that fetches it from the store).

[cjcenizal]

What's the best practice here? Should I assign the properties to ownProps instead?

[kimjoar]

In general, never mutate (so never assign to ownProps). Also, what isn't immediately clear in react-redux is that ownProps is automatically injected, so no need to pass it in here.

In the Cloud UI code base we'd do this:

import { getSections } from '../../reducers';

const mapStateToProps = state => ({
  sections: getSections(state)
});

I've added a getSections helper. What we learned through experience was that grabbing into the state here makes it more difficult to move that state around in the tree later. So now we never grab into the state outside of the reducers folder, just rely on helpers like this. That has proven very nice when "refactoring" the state layout, e.g. if you need an additional level or do some processing before returning state etc.

[kimjoar]

This is our reducers/index.js file: https://github.com/elastic/cloud/blob/master/cloud-ui/public/reducers/index.js

A tad long, but simple, at least. You can for example follow the flow for getRegion to understand how it's built up through "gradual knowledge" of the state tree: https://github.com/elastic/cloud/blob/a3557255a4e65cd51897a52640c577262845ee20/cloud-ui/public/reducers/index.js#L115

[cjcenizal]

Is there a best practice here for just passing the props along?

[kimjoar]

For the simple cases mapDispatchToProps can just be an object that lists the actions (it's just complex cases that need the functions and use of bindActionCreators). This is how we would write this file in Cloud UI:

import { connect } from 'react-redux';

import { GuideSection } from './guide_section.jsx';
import {
  openCodeViewer,
  registerSection,
  unregisterSection,
} from '../../actions';

// does not need any state, receives everything through props
const mapStateToProps = null

export default connect(
  mapStateToProps,
  {
    openCodeViewer,
    registerSection,
    unregisterSection,
  }
)(GuideSection);

(without the comment, but that was just to make it clearer)

[cjcenizal]

@kjbekkelund Thanks for the great feedback, man! Would you mind taking another look? I had a few questions about best practices.

[kimjoar]

Sweet, me likes ❤️

As far as I can see it looks 👍 now. Just some comments around how we write our containers on Cloud.

[kimjoar]

In general, never mutate (so never assign to ownProps). Also, what isn't immediately clear in react-redux is that ownProps is automatically injected, so no need to pass it in here.

In the Cloud UI code base we'd do this:

import { getSections } from '../../reducers';

const mapStateToProps = state => ({
  sections: getSections(state)
});

I've added a getSections helper. What we learned through experience was that grabbing into the state here makes it more difficult to move that state around in the tree later. So now we never grab into the state outside of the reducers folder, just rely on helpers like this. That has proven very nice when "refactoring" the state layout, e.g. if you need an additional level or do some processing before returning state etc.

[kimjoar]

This is our reducers/index.js file: https://github.com/elastic/cloud/blob/master/cloud-ui/public/reducers/index.js

A tad long, but simple, at least. You can for example follow the flow for getRegion to understand how it's built up through "gradual knowledge" of the state tree: https://github.com/elastic/cloud/blob/a3557255a4e65cd51897a52640c577262845ee20/cloud-ui/public/reducers/index.js#L115

[kimjoar]

For the simple cases mapDispatchToProps can just be an object that lists the actions (it's just complex cases that need the functions and use of bindActionCreators). This is how we would write this file in Cloud UI:

import { connect } from 'react-redux';

import { GuideSection } from './guide_section.jsx';
import {
  openCodeViewer,
  registerSection,
  unregisterSection,
} from '../../actions';

// does not need any state, receives everything through props
const mapStateToProps = null

export default connect(
  mapStateToProps,
  {
    openCodeViewer,
    registerSection,
    unregisterSection,
  }
)(GuideSection);

(without the comment, but that was just to make it clearer)

[cjcenizal]

Thanks a ton @kjbekkelund ! This has been super informative. I also really like this structure much better than the way it was before -- it's so much more flexible and transparent.