-
Notifications
You must be signed in to change notification settings - Fork 560
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 docs to use hooks #271
Conversation
I see I've done some unintended changes to the formatting as well with my prettier setup. Looks like there isn't a prettier config file present in the project - not sure what to do about those changes. They look the same rendered, so perhaps it's not a huge deal 😄 |
Great idea! Many of the examples are much shorter with hooks. Some issues though; are the new examples comparable with Preact & Inferno? Obviously React is the target framework, but that isn't the only JSX renderer. Plus, some folks may want the old class-based code for their own reasons. Possible to have both in a tabbed selector or something? Side effect; if someone is refactoring FROM classes to fucntions+hooks, both examples could help guide them. Thoughts anyone else? |
These all sounds like new features to me - features worth discussing in an issue, perhaps? 🙂 I'd love to take part of that discussion :) Preact has support for hooks, Inferno doesn't (yet). |
www/src/pages/rect.mdx
Outdated
@@ -42,36 +42,32 @@ Hook that observes and returns the measurements (ClientRect) of a DOM element. P | |||
If `observe` is false, the element's rect will no longer be observed, pass it `true` again and it will. This is mostly used for things like popovers and animations, so you can usually observe when you need to, and stop observing when you don't. | |||
|
|||
```.jsx | |||
(() => { |
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.
Why remove the iife?
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.
When there’s a single component in a code block, it will call it automatically. In my head, keeping the iife was just unneccesary syntax noise.
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 asked @ryanflorence about it and he said the reason he originally used the iife's was because react-live needs a component instance to be returned, not the component class itself. Is that not the case here?
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.
Hm okay, I don’t think that’s the case anymore at least. I’m on my phone right noe, but the gif on their readme looks like a regular function. Perhaps they invoke it for you? 🤷♂️ I’ll verify tomorrow morning 🙌
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.
Yep it's not needed anymore. You can check it yourself by going to https://ui.reach.tech/rect/ and removing the iife if you want :)
That’s fine - sorry for budding in with my comma preferences 😄 |
@selbekk I've since added a prettier config and updated some docs through merging older PRs. If you could help us clean up some of the formatting changes and merge conflict, I'll be happy to merge. I think this looks great! |
This commit adds some alt text to some placeholder images, fixes a broken link to the placecage service and removes some unnecessary markup
@chancestrickland - done son! |
This pull request goes through all the different docs, and does the following:
Example
Fixes #269