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

Improve the documentation of dataverse #116

Merged
merged 20 commits into from Apr 6, 2022
Merged

Improve the documentation of dataverse #116

merged 20 commits into from Apr 6, 2022

Conversation

fulopkovacs
Copy link
Contributor

@fulopkovacs fulopkovacs commented Apr 1, 2022

Objectives

  • Add a Get started guide
  • Improve the in-code documentation (Edit: this happened to a very small extent. We agreed on the strategy to update the in-code docs whenever someone has a question about something that is not documented well enough.)

@netlify
Copy link

netlify bot commented Apr 1, 2022

Deploy Preview for theatrejs-playground failed.

Name Link
🔨 Latest commit 233b1f9
🔍 Latest deploy log https://app.netlify.com/sites/theatrejs-playground/deploys/624abc89d410aa0007133f18

@fulopkovacs fulopkovacs changed the title Improve the documentation of dataverse Draft: Improve the documentation of dataverse Apr 1, 2022
@fulopkovacs fulopkovacs changed the title Draft: Improve the documentation of dataverse Improve the documentation of dataverse Apr 1, 2022
@fulopkovacs fulopkovacs force-pushed the add-dataverse-docs branch 2 times, most recently from 3e1c946 to 233b1f9 Compare April 4, 2022 09:38
@fulopkovacs
Copy link
Contributor Author

@AriaMinaei

This PR is intended to be production-ready. 😊

> Note that all the changes of the derivations' values inside the callback
> function (first argument of `usePrism()`) will be tracked, since `usePrism()`
> uses the `prism()` function under the hood. However, if the derivations are
> not provided in the dependency array of `usePrism()`, then it will not know if
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd say "If the prism uses a value that is not a derivation (such as a simple number, or a pointer), then you need to provide that value to the dependency array.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Okay, that does sound more clear. I'll change this.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is where the change got implemented: #116 (comment)

></div>
</>
)
}, [panelB]) // Note that `panelB` is in the dependency array
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In this example, panelB never changes

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yepp, but isn't it good practice to include it in the dependency array anyways? It is mentioned later that panelB can be omitted from the dependency array.

@AriaMinaei
Copy link
Member

How about prism.state(), prism.effect() etc?

@fulopkovacs
Copy link
Contributor Author

How about prism.state(), prism.effect() etc?

Good suggestions, I'll include them! ☺️ My original intent was to make a guide that provides information about the absolute basics that are required to get started with dataverse. I left out some things intentionally (like tickers) to keep this guide rather short, but it feels like prism.state() and prism.effect() fit here.

Comment on lines +320 to +322
second argument. If the prism uses a value that is not a derivation (such as a
simple number, or a pointer), then you need to provide that value to the
dependency array.
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@AriaMinaei This is the place where I put the change you suggested in this comment

@fulopkovacs
Copy link
Contributor Author

fulopkovacs commented Apr 6, 2022

How about prism.state(), prism.effect() etc?

These are the things that are included in the guide right now (source):
image

@AriaMinaei Do you miss anything?

Copy link
Member

@AriaMinaei AriaMinaei left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM :)

@AriaMinaei AriaMinaei merged commit 949fe93 into main Apr 6, 2022
@AriaMinaei AriaMinaei deleted the add-dataverse-docs branch April 6, 2022 15:28
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants