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.

Sign up for GitHub

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

Jump to bottom

cleanup(ts/hooks/useDetour): replace state with snapshot from createDetourMachine #2615

Merged
merged 0 commits into from
May 23, 2024
Merged

cleanup(ts/hooks/useDetour): replace state with snapshot from createDetourMachine #2615

merged 0 commits into from
May 23, 2024

Conversation

firestack
Copy link
Member

@firestack firestack self-assigned this May 21, 2024
@firestack firestack mentioned this pull request May 21, 2024
Merged
@firestack firestack marked this pull request as ready for review May 21, 2024 22:04
@firestack firestack requested a review from a team as a code owner May 21, 2024 22:04
@github-actions GitHub Actions
Copy link

Coverage of commit 8daac43

Summary coverage rate:
  lines......: 93.6% (3242 of 3465 lines)
  functions..: 73.4% (1341 of 1828 functions)
  branches...: no data found

Files changed coverage rate: n/a

Download coverage report

joshlarson
joshlarson reviewed May 21, 2024
View reviewed changes
assets/src/components/detours/diversionPage.tsx Outdated
@@ -99,7 +99,7 @@ export const DiversionPage = ({
</header>

<div className="l-diversion-page__panel bg-light">
{state === DetourState.Edit && (
{snapshot.matches({ "Detour Drawing": "Editing" }) ? (
Copy link
Contributor

Choose a reason for hiding this comment

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

Hmmm - I don't love this, because it leaks the state machine guts out of useDetour and into diversionPage. I like the (imperfect but improving) idea that useDetour's role is to tell diversionPage a bunch of facts about the detour (well, really the detour editing process) as it currently is, and I feel like calling xstate methods directly strays away from that.

I know earlier I said that we should get rid of DetourState, but having something at the level of EditScreen/SharingScreen (and eventually /RouteSelectionScreen) feels like an appropriate thing for useDetour to expose.

Copy link
Member Author

Choose a reason for hiding this comment

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

I somewhat disagree because I see the state machine replacing useDetour/useDetour being a light wrapper around the state machine. I prefer this, because to me the state machines states should ideally match what we're trying to do in the UI, so it should read well to use the state machines states directly in the UI, and IMO, exposing extra states from useDetour that the state machine is unaware of feels wrong in an anti-pattern type way (I could be wrong!), but I think that the state machine is enough of an enum that I'd argue it's typesafe enough to be part of the public api of useDetour and the UI

Copy link
Member Author

Choose a reason for hiding this comment

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

We should keep chatting about this, but the new context is #2617

@firestack firestack mentioned this pull request May 22, 2024
Merged
@firestack firestack force-pushed the kf/asn/state-machine/state-replacement branch from 8daac43 to b5c4503 Compare May 22, 2024 00:30
@firestack firestack force-pushed the kf/asn/state-machine/waypoints branch from c9f4c62 to 9dd609f Compare May 22, 2024 00:30
@firestack firestack force-pushed the kf/asn/state-machine/state-replacement branch from b5c4503 to 5fbfc26 Compare May 22, 2024 00:35
@firestack firestack force-pushed the kf/asn/state-machine/waypoints branch from 9dd609f to 0ba679d Compare May 22, 2024 00:35
@github-actions GitHub Actions
Copy link

Coverage of commit 5fbfc26

Summary coverage rate:
  lines......: 93.6% (3242 of 3465 lines)
  functions..: 73.4% (1341 of 1828 functions)
  branches...: no data found

Files changed coverage rate: n/a

Download coverage report

@github-actions GitHub Actions
Copy link

Coverage of commit 5fbfc26

Summary coverage rate:
  lines......: 93.6% (3242 of 3465 lines)
  functions..: 73.4% (1341 of 1828 functions)
  branches...: no data found

Files changed coverage rate: n/a

Download coverage report

@firestack firestack merged commit 588dcc8 into kf/asn/state-machine/waypoints May 23, 2024
@firestack firestack force-pushed the kf/asn/state-machine/waypoints branch from 0ba679d to 588dcc8 Compare May 23, 2024 12:01
@firestack firestack deleted the kf/asn/state-machine/state-replacement branch May 23, 2024 12:01
@firestack firestack force-pushed the kf/asn/state-machine/state-replacement branch from 5fbfc26 to 2cb4dbe Compare May 23, 2024 12:01
@firestack
Copy link
Member Author

Uhh, I dunno what you just did github, but I did not merge this

@firestack firestack restored the kf/asn/state-machine/state-replacement branch May 23, 2024 12:09
@firestack firestack mentioned this pull request May 23, 2024
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@joshlarson joshlarson joshlarson left review comments

Assignees

@firestack firestack

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@firestack @joshlarson