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

Update README.md #12

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
Open

Update README.md #12

wants to merge 1 commit into from

Conversation

flip111
Copy link
Contributor

@flip111 flip111 commented Jan 6, 2022

I found bits of the readme confusing.

The terminology "connected component" is unnecessary in my opinion. And is also confusing because according to the current readme a connected component just needs MonadStore and whether the connect function is used is irrelevant.

I talk a bit more about the library internals, because it seemed magic to me just reading the readme. Had to dive into the code to understand.

Not sure what the purpose is of this thing https://github.com/thomashoneyman/purescript-halogen-store/blob/main/src/Halogen/Store/Connect.purs#L56 doesn't seem to be set or modified anywhere else.

Probably my edits introduced some more mistakes, can edit some more after review.

@flip111
Copy link
Contributor Author

flip111 commented Jan 6, 2022

Have to reconsider this:

If you have a receiver set up to modify your state when you get new input, then the component will re-render on new input
But if you don’t then it won’t

@thomashoneyman
Copy link
Owner

In this case, the connect component already does have a receiver set up to send input through to your child component. And your connected component needs to receive input, or else it won't know when the store has updated. But you're correct, if you don't have a receiver then your component won't re-render on input.

@flip111
Copy link
Contributor Author

flip111 commented Jan 6, 2022

What about the original input ? is it correct what i wrote about it ?

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.

2 participants