-
Notifications
You must be signed in to change notification settings - Fork 31
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
Add Components*ReactHooks recipes #249
Add Components*ReactHooks recipes #249
Conversation
pure | ||
$ R.div_ | ||
[ R.div | ||
{ className: "box" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I used this "box"
class name because the Halogen version uses it. There's no stylesheet for either of them when I run the make
command, so maybe the style is applied in some other setting? Or was it left in the Halogen example by mistake?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Possibly by mistake?
9555246
to
756a44e
Compare
16de957
to
0767705
Compare
0767705
to
7cb13d4
Compare
If we started with ComponentsReactHooks and the task was to make a HalogenHooks equivalent, then I think we'd just use a simple rendering function and bypass all the child component complexity of our existing ComponentsHalogenHooks. So it would probably be better to come up with a slightly different use case for this example where there's clearer motivation for why the child component should track its own state that's independent from the parent. I'll think about this some more and report back if anything comes to mind. |
@milesfrain Do you think this at least shows one clear difference between Halogen and React? Those are hard to come by. |
I think the fair difference to show is that Halogen can do some more complicated stuff (child-parent communication) at the expense of more complicated code. But because this example is so basic, it just makes Halogen seem overly complex and difficult to work with. If we weren't trying to demonstrate these advanced inter-component communication techniques, we could simply mimic the React example (demo code in #253). So maybe it's not the most appropriate to clone simplified uses-cases of Halogen-only features. I didn't foresee this complication when opening #245. Ideally we'd be able to accomplish the following:
|
Hm, if we track the render count of the parent component, the advantage of the
(I wonder, with both examples, if there's a better way to track renders, but this seems to work.) So, they're not really equivalent implementations. If I were to try to get the same performance (measured by how often the parent re-renders) in React, it would be a lot trickier. |
I like the render count tracking. Maybe change the text to "Parent Renders" for clarity and add a note about that in the readme. I guess this means the Halogen recipes should also be updated in this PR too. The same "Parent Renders" indicator can also be applied to Sorry for stalling this review. I was hoping to have a moment of inspiration where a perfect example would come to mind, but that hasn't happened yet. |
No problem 🙂. I wonder if I should just trim this down to the one Let me know what you think. I'm also happy to just add "Parent Render" counts to the three existing |
I think that's the best next step. I made a typo in suggesting adding a parent render count to Given how easy it is to mix these up, I wonder if it would be worthwhile to convert to more descriptive names. That could be a separate PR, but brainstorming some ideas here for now. Feel free to suggest alternatives.
I don't know whether it would be better for the terminology to be Halogen-specific or more general. For example
I think it's still nice to have full parity between the react and halogen recipes, so would be in favor of keeping all three. |
Could we keep this as one recipe, but split each component into its own part and run all three of them in the same main :: Effect Unit
main = do
runhalogen1
runReact1
runHalogen2
runReact2 ? |
There will likely need to be some DOM finagling to create the necessary nodes to launch each application (rather than just attaching directly to Pro:
Con:
Unknown:
|
Specifically to the point about bloated recipe list - that's been on my mind a bit recently as i've been making extensive use of the recipe list to help me write Halogen Hooks things for the first time - i think that the solution to too many recipes would be better done by enabling filtering of a long list of small recipes rather than bundling recipes together. i say this because the big win with recipe list (and, as i say, i've personally been benefitting from it recently) is enabling you to isolate the core code that tackles some facet of a larger problem. Note that this doesn't have to be done on the README itself - we could make a nice little app that uses the same readme but offers some affordances (ie tags, checkboxes, ordering etc) to enable a person to quickly zero in on a recipe that helps them. That recipe app could itself be a recipe! |
This would be great. Could share some code between this and the interactive table of container APIs that's also on my wishlist. |
@afcondon Thanks for the comment about the bloated recipe list. I agree with you that the bloat problem is better solved by presenting the list in a different manner rather than merging recipes together. So, let's not merge these recipes into one, but keep them separate. |
I've added a parent render count to each of those six components. I hadn't realized until I started work on it that the only difference in behavior between the React and Halogen implementations is with the There also might be a concern that adding the render counter adds a |
Thanks for your work on this @ptrfrncsmrph! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
pure | ||
$ R.div_ | ||
[ R.div | ||
{ className: "box" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Possibly by mistake?
Addresses #245.
I mostly translated the existing
Components*HalogenHooks
recipes. I think there's generally a benefit to being able to compare what amounts to the same UI in Halogen hooks and React hooks, but in this case it seems like the Halogen examples are meant to showcase how to communicate state from child to parent component. In React, that's not really possible, and in my implementation, all state is held in the parent and setter/updater functions are passed down to the children along with the state value. So, although I think this is how the examples would translate to idiomatic React, the motivation for implementing these particular UIs might not be as clear as it is with Halogen.