Conversation
| onStatusChange = noop, | ||
| onOnline = noop, | ||
| onOffline = noop | ||
| }) { |
There was a problem hiding this comment.
| }) { | |
| } = {}) { |
This change would make the param optional
|
Not sure bringing a UI library in is a good idea, I would say these hooks should remain as small as possible to allow people consuming them to decide how they want to. ie, if I'm not using evergreen, or don't like the look of these popups .. I can't really use this hook |
hanford
left a comment
hanford
left a comment
There was a problem hiding this comment.
alternatively you could create a hook and open source it under your own name with this functionality.
I would say bringing in evergreen ui might be out of scope for these hooks
|
thoughts @jamiebuilds @fouad ? |
|
Yeah I agree I added that while developing. Need opinions on showing how to
use the hooks
…On Fri, 26 Oct 2018 at 23.41 Jack Hanford ***@***.***> wrote:
***@***.**** requested changes on this pull request.
alternatively you could create a hook and open source it under your own
name with this functionality.
I would say bringing in evergreen ui might be out of scope for these hooks
—
You are receiving this because you authored the thread.
Reply to this email directly, view it on GitHub
<#6 (review)>,
or mute the thread
<https://github.com/notifications/unsubscribe-auth/ADkzMk6cuil2h4BgTN6kFlgpbbkaqiYiks5uoztLgaJpZM4X77yV>
.
|
|
Yeah, no need for UI, it's only going to make things more confusing |
|
Definitely no UI 😛 |
package.json
Outdated
| "devDependencies": { | ||
| "ava": "^0.25.0", | ||
| "browser-env": "^3.2.5", | ||
| "evergreen-ui": "^4.2.0", |
There was a problem hiding this comment.
Seems a bit overkill to pull in a UI library
| function MyComponent() { | ||
| let onlineStatus = useOnlineStatus(); | ||
| let onlineStatus = useOnlineStatus({ | ||
| onStatusChange: online => |
There was a problem hiding this comment.
I feel like this should be implemented as something like <OnlineToaster status={onlineStatus} /> that triggers a toast on prop change rather than introducing a new concept to Hooks
There was a problem hiding this comment.
This isn't really a new concept for hooks useImperativeMethods is basically this.
I think it's fine, but I would assume most of the time you'd use it for setX calls
There was a problem hiding this comment.
@fouad I can see the point you are making here. I added those hooks because I sometimes need to call some function whenever the online status changes. What do you people think? Should this be implemented here or not? 🙏
|
Just re-read the messages. I think @hanford mistook me for adding a UI lib for the main bundle. I didn't, and just used the |
This is giving the users more flexibility so they can pass their own hooks to the |
|
I've removed the ui dependency and opt for a |
6 participants
onlineeventofflineeventonlineandofflineevent