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

Documentation: A slight revamp of the README, wording, and clearer links #3458

Merged
merged 12 commits into from
Jul 22, 2022
Merged

Documentation: A slight revamp of the README, wording, and clearer links #3458

merged 12 commits into from
Jul 22, 2022

Conversation

egilll
Copy link
Contributor

@egilll egilll commented Jul 9, 2022

Hi. This pull request changes a handful of things which I found to be confusing when reading this otherwise excellent documentation. I apologize if this PR is too bold — do of course feel free to close this PR should you disagree with these changes.

The changes are:

  • I found the lack of a link to the API in the sidebar to be extremely confusing. It turns out that it was in the header, but it still always takes me a while to remember where to look for it.
  • The API docs include links to more detailed articles on some topics, but that link is "hidden" behind the "Usage:" label. I did not figure that out at first, and so I've changed the link to a "(further information)" label.
  • In "Creating custom observables", it was not clear that "the concept of atoms" refers to a class used interally by MobX.
  • README:
    • The main README in GitHub did not include a link to the main documentation at mobx.js.org which I occasionally found confusing.
    • I believe it is better to let readers know what the project is about before showing a long list of sponsors, so I've moved the introduction above the list of sponsors.
    • I removed the "What others are saying" section. The quotes were not convincing and fairly childish, which I associate mainly with brand new, non-stable projects.
    • Changed the link to the MobX book from an incomplete Google Books preview to PacktPub/Amazon.
    • Changed the term "Transparent Functional Reactive Programming" to simply "functional reactive programming". The README read as if TFRP is some general term one could be expected to know, but no, it is practically non-existent, not being used by anyone apart from MobX and two (low-impact) papers. "Transparent Reactive Programming" was used internally by someone at Meteor but that term did not catch on either. In my view it is therefore not helpful to readers to include it.
  • Removed the sentence "Collections such as arrays, Maps and Sets are made observable automatically" from the page "Creating observable state". It appears that this claim is missing some additional context.
  • Added "(£5)" to "Download the Mobx cheat sheet".
  • Also some minor wording fixes.

@changeset-bot
Copy link

changeset-bot bot commented Jul 9, 2022

🦋 Changeset detected

Latest commit: 63f1324

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 2 packages
Name Type
mobx Patch
mobx-react Patch

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

docs/observable-state.md Outdated Show resolved Hide resolved
…d to makeObservable. Fixing the spelling of "overriden"→"overridden".
@kubk
Copy link
Collaborator

kubk commented Jul 18, 2022

First of all, thank you for your initiative and will to improve things, it's appreciated. The issue is that PR is too big and it'd have been merged more quickly if it had been split into different PRs. But we may continue as is, it'll just take more time

website/static/css/custom.css Show resolved Hide resolved
docs/api.md Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
@mweststrate
Copy link
Member

All changes sound good to me, thanks for making this effort! The only one I have a beef with is this one:

Changed the term "Transparent Functional Reactive Programming" to simply "functional reactive programming". The README read as if TFRP is some general term one could be expected to know, but no, it is practically non-existent, not being used by anyone apart from MobX and two (low-impact) papers. "Transparent Reactive Programming" was used internally by someone at Meteor but that term did not catch on either. In my view it is therefore not helpful to readers to include it.

What mobx does is not strictly functional reactive programming, and just RP itself is very wide and vague as well. It's the transparent nature that make MobX quite stands out, so I think it is fine to keep this term, even when it is new for most people and has little prior art, it kinda covers the growing niche we're in better than the others. Checking Svelte and Solid, they just call it "Reactive", without any additional flavours :-P

@egilll
Copy link
Contributor Author

egilll commented Jul 19, 2022

I have reverted back to the original wording.

I did originally have a hard time finding information about the term so perhaps either of the following would work:

but MobX brings what it refers to as transparent functional reactive programming to the next level

but MobX brings transparent functional reactive programming (a concept which is further explained in the MobX book) to the next level

@mweststrate
Copy link
Member

Both work for me, 2nd is slightly cooleer I think

@urugator urugator closed this Jul 21, 2022
@urugator urugator reopened this Jul 21, 2022
@kubk
Copy link
Collaborator

kubk commented Jul 21, 2022

I see that CI is failing, probably due to outdated snapshots. I'll fix it once I am available

@kubk
Copy link
Collaborator

kubk commented Jul 22, 2022

@egilll Is option Allow edits from maintainers enabled for this PR? Could you enable it?

@kubk kubk merged commit 78d1aa2 into mobxjs:main Jul 22, 2022
@kubk
Copy link
Collaborator

kubk commented Jul 22, 2022

@egilll Thank you so much 👍

@github-actions github-actions bot mentioned this pull request Jul 22, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants