Skip to content
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] Improve state machine separation of concerns #550

Merged
merged 1 commit into from Mar 18, 2021

Conversation

benoitgrelard
Copy link
Collaborator

@benoitgrelard benoitgrelard commented Mar 11, 2021

Sorry, it mostly looks like completely new code now because I moved stuff into 2 separate files 😒
I was finding it hard to follow before, so now we have one file for the state machine implementation and one for the actual tooltip statechart.


  • updated naming conventions to match xstate closer (I had camelCase vs. UPPER_CASE the wrong way around for state vs. events)
  • decoupled context updating from events using similar assign concept from xstate
  • I am still injecting send from entry/exit actions, which apparently is not really correct looking at xstate

Note: overall this implementation is still far from the decoupling that xstate has because they distinguish the concept of a pure machine vs. an interpreter. Ours is conflated basically, but I'm not sure it's worth really pushing further. I experimented today but then I feel like I'm just re-implementing a fully featured state machine library and probably doing it wrong too…

);
}
} else {
PREVIOUS_STATE = CURRENT_STATE;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I know you haven't changed this but it really confused me seeing uppercase variables being reassigned like this. I would usually expect all uppercase to be a constant.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good point. Is it ok with you if I make this change once we've merged the stack of PRs to make conflict handling easier?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

sure no prob πŸ‘

@benoitgrelard benoitgrelard merged commit b95e3c1 into main Mar 18, 2021
@benoitgrelard benoitgrelard deleted the tooltip-machine branch March 18, 2021 14:16
benoitgrelard added a commit that referenced this pull request Mar 22, 2021
# 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)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants