Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

UI demo type components: workspace view and itemscontainer #32

Merged
merged 14 commits into from
Oct 12, 2019
38 changes: 20 additions & 18 deletions docs/ADR/adr-28-08.md
Original file line number Diff line number Diff line change
@@ -1,18 +1,18 @@
# Relationship between Storybook and DPE Client

* Status: accepted
* Deciders: Alli and Eimi
* Date: 2019-08-28
- Status: accepted
- Deciders: Alli and Eimi
- Date: 2019-08-28

Technical Story: N/A

## Context and Problem Statement

We needed to clarify the relationship between the DPE Client repository, where the components we are using to populate the Storybook repo have already been written, and the Storybook repository.
We needed to clarify the relationship between the DPE Client repository, where the components we are using to populate the Storybook repo have already been written, and the Storybook repository.

+ From which repo would components be published?
+ Which repos would consume components from NPM?
+ Should the Storybook live inside the Client repo?
- From which repo would components be published?
- Which repos would consume components from NPM?
- Should the Storybook live inside the Client repo?

## Decision Drivers

Expand All @@ -26,39 +26,41 @@ N/A

## Decision Outcome

Chosen option: Option 2, because this allows us to refactor components' code and preview changes within the Storybook locally — before publishing the component to the hosted Storybook and NPM.
Chosen option: Option 2, because this allows us to refactor components' code and preview changes within the Storybook locally — before publishing the component to the hosted Storybook and NPM.

This means that our workflow for populating the Storybook and refactoring the Client code is as follows:

1. Duplicate component code to Storybook repo
2. Publish completed components to NPM
3. Remove the original component code from the Client and import via NPM

### Positive Consequences
### Positive Consequences

### Negative consequences
Caveat: If more than one person is working on the Storybook and DPE Client, they'll need to sync up to ensure that details in code refactors are not lost due to overlapping work.
### Negative consequences

Caveat: If more than one person is working on the Storybook and DPE Client, they'll need to sync up to ensure that details in code refactors are not lost due to overlapping work.

If possible, also avoid having people working simultaneously on a component that consumes / is consumed by another component (i.e., one person working on a card component and another person working on a list component that consumes card components).

## Pros and Cons of the Options
## Pros and Cons of the Options

### Option 1: Publish components to NPM from the Client and consume via Storybook

This workflow would mean that we would need to be refactoring code in the client before publishing individual components to NPM. To modify the components in the Storybook, we would need to re-publish to NPM from the client. This is gross.
This workflow would mean that we would need to be refactoring code in the client before publishing individual components to NPM. To modify the components in the Storybook, we would need to re-publish to NPM from the client. This is gross.

### Option 2: Publish components to NPM from the Storybook and refactor to consume in the Client

Although this workflow means we are essentially copy-pasting code over from the Client repo to the Storybook, it allows us to:

+ Refactor component code while previewing the Storybook locally
+ Reflect changes to the code in the Client by refactoring to import components from NPM
- Refactor component code while previewing the Storybook locally
- Reflect changes to the code in the Client by refactoring to import components from NPM

This is a more sensible workflow than option one.
This is a more sensible workflow than option one.

### Option 3: Same repo for both Storybook and Client

We didn't discuss this option in detail — mainly because it would likely introduce too many moving parts to the same repo.

## Links
N/A
## Links

N/A
9 changes: 4 additions & 5 deletions docs/ADR/reason-09-11.md
Original file line number Diff line number Diff line change
@@ -1,8 +1,8 @@
# Storybook Business Decision

* Status: Accepted
* Deciders: Pietro and Eimi
* Date: 2019-08 (Sometime)
- Status: Accepted
- Deciders: Pietro and Eimi
- Date: 2019-08 (Sometime)

## Context and Problem Statement

Expand All @@ -20,8 +20,7 @@ Currently we have to clone and run local servers to demonstrate the components i

Overly complicated components are difficult to test. We need to break them down and isolate the components so that it is easier to understand and extend. We can also easily define the interface of the components. This will help people contribute and maintain DPE.

Additionally this [post](https://www.conductor.com/nightlight/how-to-use-react-storybook/
) explains anti-pattern scenarios that we want to avoid:
Additionally this [post](https://www.conductor.com/nightlight/how-to-use-react-storybook/) explains anti-pattern scenarios that we want to avoid:

> Here's a typical situation: you're going through the specs of a new page that you have to develop. All the components look familiar and you've seen them used elsewhere. You plan your tasks based on an understanding that some components have already been implemented. After agreeing on the timing and starting a new sprint, you dig around in the code a bit and are horrified to realize that the existing components are not appropriate for reuse. Now you have three options, none of which are great:
>
Expand Down
76 changes: 76 additions & 0 deletions docs/guides/error-invariant-multiple-react-router-dom-09-28.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,76 @@
# Error: Invariant failed

## Context and Problem Statement

Turns out that this error "Error: Invariant failed" is because how [React Router v4](https://www.sitepoint.com/react-router-v4-complete-guide/) and [React Router v5]() is different. With the new v5 there can only be one Router, because it's using the new React Context API to do some routing. See here for another [explanation](https://gist.github.com/StringEpsilon/88c7b049c891425232aaf88e7c882e05#explanation)

Why we need two Routers is that Breadcrumb component has LinkContainer, which depends on React-Router-Dom. So, when the component is published, it then also bundles the React-Router dependency. When we import the Breadcrumb, since we already have a React-Router in the main DPE-client repository, it creates two Contexts in React. The routes created using a different Context cannot be found by React, and so it throws the `Invariant failed: You should not use <Route> outside a <Router>`.

## Suggestions

### Use the Router + Route

From [this](https://github.com/marmelab/react-admin/issues/3078) this is how it was recommended, but since the Breadcrumb was nested it's not a straightforward fix.

```jsx
<BrowserRouter>
<div>
<Route path="/" component={} />
</div>
</BrowserRouter>
```

### Use Webpack to remove dupes in Client

Was also advised to do [this](https://gist.github.com/StringEpsilon/88c7b049c891425232aaf88e7c882e05) but since our setup is CRA in DPE-client, it's quite a few hurdles to jump. If you have webpack, from the [webpack](https://webpack.js.org/guides/code-splitting/#prevent-duplication) is useful to understand to prevent dupes.

### peerDependencies

[Yarn](https://yarnpkg.com/lang/en/docs/dependency-types/) website says:
> Peer dependencies are a special type of dependency that would only ever come up if you were publishing your own package.
> Having a peer dependency means that your package needs a dependency that is the same exact dependency as the person installing your package. This is useful for packages like react that need to have a single copy of react-dom that is also used by the person installing it.

This also wouldn't work due to us actively developing storybook. It doesn't prevent webpack from packaging in the react-routers.

### Solution: Update Storybook webpack to remove react, react-router and react-router-dom

You can prevent Webpack to bundle things that should not be duped.
See package.json:

```js
resolve: {
alias: {
'react': path.resolve(__dirname, './node_modules/react'),
'react-dom': path.resolve(__dirname, './node_modules/react-dom'),
'react-router': path.resolve(__dirname, './node_modules/react-router'),
'react-router-dom': path.resolve(__dirname, './node_modules/react-router-dom')
}
},
externals: {
// Don't bundle react or react-dom or react-router
react: {
commonjs: 'react',
commonjs2: 'react',
amd: 'React',
root: 'React'
},
'react-dom': {
commonjs: 'react-dom',
commonjs2: 'react-dom',
amd: 'ReactDOM',
root: 'ReactDOM'
},
'react-router': {
commonjs: 'react-router',
commonjs2: 'react-router',
amd: 'ReactRouter',
root: 'ReactRouter'
},
'react-router-dom': {
commonjs: 'react-router-dom',
commonjs2: 'react-router-dom',
amd: 'ReactRouterDOM',
root: 'ReactRouterDOM'
}
}
```
Loading