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
[Tooltip] Add custom duration #551
Conversation
@@ -71,7 +65,8 @@ const tooltipStateChart: TooltipStateChart = { | |||
(event, context, send) => { | |||
restTimerId = window.setTimeout( | |||
() => send({ type: 'REST_TIMER_ELAPSE', id: event.id }), | |||
REST_THRESHOLD_DURATION | |||
// @ts-ignore | |||
event.restDuration |
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 know how to restrict the type further here so it knows which event it is…
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've been thinking about this and realising I wasn't fully understanding things when we spoke on the call. I'm now wondering why we need to pass the durations through every MOUSE_ENTER
or MOUSE_MOVE
event.
I would usually expect something like:
const { restDuration, bypassRestDuration, ...restProps } = props;
React.useEffect(() => {
send({ type: 'UPDATE_REST_DURATION', restDuration });
}, [send, restDuration]);
React.useEffect(() => {
send({ type: 'UPDATE_BYPASS_REST_DURATION', bypassRestDuration });
}, [send, bypassRestDuration]);
Then the machine would have the following (these are a good example of events that don't transition):
on: {
UPDATE_REST_DURATION: { actions: [assignRestDuration] },
UPDATE_BYPASS_REST_DURATION: { actions: [assignBypassRestDuration] },
}
And your waitingForRest
would do:
entry: [(_, context, send) => {
restTimerId = window.setTimeout(() => send(), context.restDuration);
}]
This would alleviate type issues I believe.
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.
It seems strange to have events just to update the context though I guess.
But also I don't think that would work, because if we did that, every single instance of tooltip would override each other because there's one shared state machine for all tooltips.
We do want these timings to change on the fly based on the tooltip currently being used.
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.
It seems strange to have events just to update the context though I guess.
Think my naming confused the sitch. The event could be PROP_CHANGE
/ REST_DURATION_CHANGE
, or whatever you wanna call it. State machine events don't have to map to DOM events, they're just "things that happened".
We do want these timings to change on the fly based on the tooltip currently being used.
Ah of course! That was the missing piece of the puzzle, cheers. Agree we should leave as is in that case 🙂
56a42a6
to
80cbded
Compare
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.
Approving now that you've built on top of this more in a separate one 👍
# New features Breaking changes are indicated with a 🔥 icon. [Tooltip] Add custom duration support (#550, #551, #554, #558) [Tooltip] Add unmount animation support (#558) [Toggle] Rename `ToggleButton` primitive to `Toggle` and `toggled` to `pressed` (#546) 🔥 [ToggleGroup] New primitive! (#376) 🎉 # Fixes [ContextMenu] Ensure you can open a context menu when one is already open (#565) [Popper] Fix potential issues with non-measured positioning (#541) [Presence] Fix unmount hanging (#548)
main
once [Tooltip] Improve state machine separation of concerns #550 is merged.I'll add versions file after the above is merged ☝️
Closes #543