-
Notifications
You must be signed in to change notification settings - Fork 3
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
feat(use-presence): allow use-presence to be driven by arbitrary data #11
Conversation
Hey, thanks a lot for sharing your idea—I think that's certainly interesting! The use case would be to have an array of items where only one of them is visible at a time, right? E.g. a tab navigation where the content is animated. Maybe it would make sense to implement it as a wrapper hook indeed? We could still export it from this package if you like: import {useItemPresence} from 'use-presence'; The intention here would be to keep the size of the original Out of curiosity: How did you learn about the |
Hey @amannn, your assumption about the use case is exactly right! If you have an array/record/map where you want to be able to swap the data in & out arbitrarily and see the old data maintained while a component displaying it animates out and then update the data to whatever the latest is to animate a new piece of data back in. I think there's a valid take that this would make more sense as a wrapper hook. That's how I originally implemented it in my use-case and it works well. Adding the lag-data state also forces a few more renders to fire. It's never been a problem in my use-cases, but I can see why you wouldn't want to make your users have to deal with that if they don't need it. I've updated the PR to create and export a separate hook. If you like this as it is, then just let me know and I'll write up the documentation! And I found |
…the option types for the original hook change, the wrapper hook will receive the update by default
…alues from usePresence, and export both of their options types
Little extra update: because the wrapper hook is using the type of the options from the base hook, I just separated out the types for each of their options argument. That way if the options type for I also exported both of these types. I did this because when I used this hook in my project, I had a import usePresenceBase from "use-presence";
export const usePresence = (
isVisible: boolean,
options: Parameters<typeof usePresenceBase>[1] = { transitionDuration: 150 }
) => usePresence(isVisible, options); But once the types are exported I can use |
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.
Hey, thanks a lot for the update here! I've left some comments inline, let me know what you think!
packages/use-presence/src/index.tsx
Outdated
@@ -12,20 +12,22 @@ type SeparateTransitionConfig = { | |||
exitTransitionDuration: number; | |||
}; | |||
|
|||
export type UsePresenceOptions = ( |
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.
Hmm, would it bother you much if we don't export this for now? I always find the naming of this quite hard (UsePresenceOptions
, PresenceOptions
, TransitionConfig
, …). As long as it's private it's not that important. As you've mentioned there are ways in TypeScript how types can be generated based on a function signature.
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.
Alrighty, I walked back those updates 👍
packages/use-presence/src/index.tsx
Outdated
@@ -107,3 +109,57 @@ export default function usePresence( | |||
isExiting | |||
}; | |||
} | |||
|
|||
export type UseUniqueDataPresenceOptions<T> = { |
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.
Maybe we should move the hooks to separate files. I'm not entirely sure to be honest, but maybe some bundlers would benefit from that in regards to tree shaking. We can still export both from an index.tsx
file. At least from a maintainability perspective that might be handy.
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.
Done, now there's use-presence.tsx
, use-presence-switch.tsx
, and a barrel index.tsx
👍
packages/use-presence/src/index.tsx
Outdated
return Boolean(a); | ||
} | ||
|
||
export function useUniqueDataPresence<T>( |
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.
Bike-shedding a bit on the name here:
usePresenceSwitch
useSwitchPresence
useItemPresence
I kind of like the term "switch", since it expresses the concept that there can be multiple options but only one active at a time. react-router
also uses the term.
I think my preference would be usePresenceSwitch
—what do you think?
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 also like usePresenceSwitch
though I also went with updating data
& dataInput
to item
& latestItem
. So maybe now you might prefer useItemPresence
but I think both names together are still self explanatory, but let me know how you feel about it now 👍
packages/use-presence/src/index.tsx
Outdated
isMounted: isMounted && validationCheck(data), | ||
data | ||
}; | ||
} |
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.
Can you add some tests for the new hook? The existing tests might help for inspiration. Also here moving to separate files per hook might be a good idea organization-wise.
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.
Done! Added to a separate test file packages/use-presence/test/use-presence-switch.test.tsx
. I made them pretty lightweight as I didn't want to retest underlying functionality from the usePresence
base hook. 👍
packages/use-presence/src/index.tsx
Outdated
} | ||
|
||
export function useUniqueDataPresence<T>( | ||
dataInput: T, |
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.
Maybe we could use item
or value
as a name to refer to individual items? I personally also use expressive names for generics, I think that helps to understand the intent a bit better on the consumer side.
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 like item
, updated 👍
packages/use-presence/src/index.tsx
Outdated
export function useUniqueDataPresence<T>( | ||
dataInput: T, | ||
{ | ||
equalityCheck = defaultEqualityCheck, |
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.
Do we need to make this configurable? If we just assume reference equality, I think the consumer could implement a special equality check outside of the lib and achieve the same (e.g. with useMemo
).
For libraries I'm largely in favour of avoiding options, especially if they can be solved outside. There's always a use case popping up at some point that an API didn't consider and if it can be achieved without changing the lib, I think that's useful 🙂.
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 don't need it to be configurable in any of my use-cases. I also agree that this logic can be moved outside of the library where it's needed. But reference checks meet the needs of all my scenarios 👍
packages/use-presence/src/index.tsx
Outdated
dataInput: T, | ||
{ | ||
equalityCheck = defaultEqualityCheck, | ||
validationCheck = defaultValidationCheck, |
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.
Here the same question if we can avoid it. Is this intended for the hook to be called with values that are not an item? I think we can just check for != null
, if the user needs more customization, also this can be customized by transforming the value that's passed to the hook. Also I think false
can likely be a valid value.
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 switched to a non-configurable isNil
function to validate because I wanted to check item === null || item === undefined
, as that is somewhat important in the cases I've run into ℹ️
… separate files and performs some name updates and configuration and type remodeling
@amannn I've responded now to all inline comments. For everything that I went exactly along with, there's a 👍 and for the one comment that I went slightly off script there's a ℹ️ . Let me know what you think at this point and if you like where the code has ended up and you feel that it's well tested then I will add some documentation for the new hook! |
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.
Would you like to add a small section to the README to introduce the new feature? Maybe we should think about a good example for this feature. E.g. a tab navigation comes to my mind, where the tab content animates in and out.
packages/use-presence/src/index.ts
Outdated
@@ -0,0 +1,5 @@ | |||
import usePresence from './use-presence'; | |||
|
|||
export * from './use-presence-switch'; |
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 guess we could just use default exports for both files to be consistent, right?
There's btw. a one-liner for reexporting:
export {default} from './usePresence';
export {default as usePresenceSwitch} from './usePresenceSwitch';
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.
Will do 👍
import usePresence from './use-presence'; | ||
|
||
export function usePresenceSwitch<ItemType>( | ||
latestItem: ItemType, |
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.
Maybe just item
? I'd care more about the naming of this over the internal state variable, as this is what the consumer will see in autocomplete.
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.
item
& mountedItem
works fine for me 👍
} | ||
|
||
function isNil<ItemType>(value: ItemType) { | ||
return value === null || value === undefined; |
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.
A shortcut for this is value == null
, but fine by me if you prefer it more explicit.
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 personally have not used !=
or ==
in JavaScript basically ever. But this seems like a fine enough use case for me as long as it remains contained in this function. Otherwise it would be too easy to accidentally type !==
or ===
. 👍
return { | ||
...otherStates, | ||
isMounted: isMounted && !isNil(item), | ||
item |
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.
Should we call this mountedItem
in relation to the existing isMounted
state?
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.
item
& mountedItem
works fine for me 👍
getByText('initial value'); | ||
await waitFor(() => getByTestId('isVisible, isMounted')); | ||
getByText('re-assigned value'); | ||
}); |
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.
Cool! 🙌
If you feel like it we could test some more states like unmounting completely (like your initial demo allows it), but up to you!
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.
Done! 👍
@amannn Responded to all comments in agreement (👍) and updated! Added docs in the commit after, writing is not my strong suit so just let me know what you think! |
Thanks a lot, looks great! 👏 Thanks also for drafting a section for the docs! Would you mind if I try to rephrase it a bit? |
@amannn you absolutely may rephrase it. I don't know if you want to just to inline recommendations in the PR and I can commit it. Or if you want to commit it to my fork, I just added you as a contributor, and I have allowing edits from maintainers checked off. |
Great, thanks! I've created a PR to propose my changes: EthanStandel#1 |
@amannn I left a couple comments on that, but did approve it for the record. You should be good to merge at any time. Once you do, then I assume you'll be ready to merge this PR. I'll leave this one as is unless I hear any other feedback from you 👍 |
feat: `usePresenceSwitch` cleanup
Released in |
The purpose of this change is to solve another challenging problem with presence animations within the already available hook which, in my opinion, makes it even more useful!
With this change,
usePresence
can take an argument of arbitrary type, and it will manage a staggered state of that argument. This allows the hook to function as it always did withboolean
values, but if it receives another type then it will manage the render state based upon if the value is truthy or not+1. However, if the state changes between two truthy values, then the added change will ensure that the original data continues to persist until the element has transitioned out, at which point the element will transition back in with the new data without ever unmounting.Here is a StackBlitz example showing a simple use-case for the new version of the hook alongside the old behavior working as expected.
number
and you want0
to be truthy. If you think this is important I can also add that.