The Wayback Machine - https://web.archive.org/web/20210124161455/https://github.com/mobxjs/mobx-react/issues/677
Skip to content
This repository has been archived by the owner. It is now read-only.

Shared user guide #677

Closed
FredyC opened this issue May 29, 2019 · 5 comments
Closed

Shared user guide #677

FredyC opened this issue May 29, 2019 · 5 comments

Comments

@FredyC
Copy link
Contributor

@FredyC FredyC commented May 29, 2019

@mweststrate With 6.0 out now (👏), would you consider trimming down README and consolidate that at the docs site I've made? I still haven't got any feedback about that site from you.

https://mobx-react.js.org

@mweststrate
Copy link
Member

@mweststrate mweststrate commented May 31, 2019

@FredyC FredyC pinned this issue Jun 8, 2019
@FredyC FredyC unpinned this issue Jun 8, 2019
@mweststrate
Copy link
Member

@mweststrate mweststrate commented Jun 19, 2019

Only glanced over it, but some feedback points:

  1. I think having a third place for documentation for mobx-react is a bad idea. Two (mobx.js.org and the readme of this repository itself) is basically already bad enough. I think the documentation proposed here should be a mobx+react recipes page on the official docs (between section 8.0 and 8.1?). And then forward to it from the observer page. Some of the sections have quite some overlap with the general documentation and could be skipped I think
  2. Beware of being dismissive about class based components. We are not a React style police, and people will be stuck with them for the coming years in large existing code bases. There is no need to alienate those people just because they aren't running a greenfield project. The page / docs in general are written primarily from a mobx-react-lite perspective. That is ok, but the domain on which it is hosted suggests otherwise :)
  3. I think the primary point should be to explain how to achieve certain goals that might developers have in certain situtations: such as external state, component local state, and side effects both with hooks and classes. And furthermore elaborate as done on the subtile differences between the HoC, hook and wrapper.

Some of the example are a bit convoluted, for example on the introduction page:

Creating an inline component is quite a bad idea, as it kills the reconciler, and at least a bad example to set: const renderTodo = (done: boolean) => todo => (. I suggest to extract it to a separate component and make it an observer. Which is consistent with the rest of the MobX docs: renderers for list items should be separate components

The localStore's addTodo is expressed in terms of the ref. If instead the text to be added would be passed in, there would be a clear separation between the logic and the internal stuff of the component, and the entire initializer could actually be extracted out of the component.

The example uses createTodos, but it seemst to be defined nowhere?

So I think there could be a bit simpler example to introduce the whole concept :). Random idea: https://codesandbox.io/s/evwj5. Yes, it is more contrived, but it will have less noise so it is easier to see what it is all about


All IMHO of course, awesome that you are taking the effort!

The beta notice on the libraries page can be removed.

Sections I really liked:

@FredyC
Copy link
Contributor Author

@FredyC FredyC commented Jun 19, 2019

Thanks for the feedback!

  1. I think having a third place for documentation for mobx-react is a bad idea.

The main motivation was to minimize duplicate information in mobx-react a mobx-react-lite readme. I wanted to consolidate that so people can really come to one place no matter what lib they are using. I am sure it still needs improvements, but without actual feedback, it's harder for me to see what needs to be changed. Also, the mobx.js.org could remove mention of React (doesn't really fit there).

2. Beware of being dismissive about class based components. We are not a React style police, and people will be stuck with them for the coming years in large existing code bases.

I wonder where I am actually dismissing class components as something less? It definitely wasn't the intention. Personally, I haven't been using classes even before hooks unless I really had to. I would of course love if people would just abandon classes, so I am sometimes biased there, but I am trying not to be.

I think I want to figure out some nice way how can user switch the mode to either hooks or classes and see that page for that respective approach. I don't want to mingle both in a single document, that would be way too confusing. Besides, when classes will be really gone, I want to just turn off the switch without updating those docs and removing all mentions manually.

Creating an inline component is quite a bad idea, as it kills the reconciler, and at least a bad example to set

Defining inline function renderSomething is not an inline component. It is called as a function, not wrapped into React.createElement. I don't find anything wrong about that practice, it's just way to structure the code better. Nesting with inlined array.map is awful imo.

The localStore's addTodo is expressed in terms of the ref. If instead the text to be added would be passed in, there would be a clear separation between the logic and the internal stuff of the component, and the entire initializer could actually be extracted out of the component.

Well, the intention was to show that actions on the store are auto bound so they can be used directly as a callback prop without extra function wrapping or binding. Semantically, it definitely makes sense what you are suggesting.

The example uses createTodos, but it seemst to be defined nowhere?

That's the slight problem of Docz. All those live examples are not linked to that code shown below them. It's necessary to synchronize it manually. I need to figure some other way to tackle that more gracefully.

So I think there could be a bit simpler example to introduce the whole concept :).

Right, the timer example is nice, I will use that :)

The beta notice on the libraries page can be removed.

Will do, although it clearly states it applied in May 2019.

@mweststrate
Copy link
Member

@mweststrate mweststrate commented Jun 20, 2019

[offtopic]
@FredyC could you PM or mail (info@michel.codes) me for a personal question? Noticed that PMs are disabled on your twitter account :)
[/offtopic]

Also, the mobx.js.org could remove mention of React (doesn't really fit there).

Don't agree on that one; first of all, mobx-react is the primary use case of mobx. Secondly, without concrete application in the forms of mobx-react, the mobx documentation would be very abstract in general, and describing artificial usecases and problems most people don't have.

I do agree on the consolidation though. I would be perfectly fine by making an separate top level section on mobx.js.org, and emptying and forwarding the readme of mobx-react and mobx-react-lite there.

I don't want to mingle both in a single document

I see. I think having separate pages, describing the same use cases would be very clear as well. Than we can just cross link and people can compare if they want, and also we can recommend the hooks version on the class page.

It is called as a function

Yeah, the problem is in either case, that it will re-render the full list on every change. Smaller components perform much better (as it explained later in the docs). But yeah, an simpler example avoids this problem altogether and we need to to introduce less concepts at once (I mean, people shouldn't need to think about performance in the first example, but not unconsciously be learning a bad practice either)

Well, the intention was to show that actions on the store are auto bound so

Good point! Missed that. I think the timer has this case as well.

@mweststrate
Copy link
Member

@mweststrate mweststrate commented Aug 27, 2020

Closing as combined docs are WIP on the mobx 6 branch: https://deploy-preview-2327--mobx-docs.netlify.app/react/react-integration.html

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
2 participants