search

tldraw / tldraw

SDK for creating whiteboards and canvas experiences on the web.
https://tldraw.dev
Other
33.88k stars 2.03k forks source link

[feature] ui events #1326

Closed steveruizok closed 1 year ago

steveruizok commented 1 year ago

This PR updates the editor events:

Release Note

vercel[bot] commented 1 year ago

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Updated (UTC)
examples ✅ Ready (Inspect) Visit Preview May 11, 2023 10:08pm
tldraw-docs-staging ✅ Ready (Inspect) Visit Preview May 11, 2023 10:08pm
orangemug commented 1 year ago

I don't think this is the right approach (if I understand correctly). The previous branch used the core data structures to avoid losing changes, and making you think about events every time you make a change. For example

https://github.com/tldraw/tldraw/blob/2d8da6a1b037bfe88a261a94104b987b4960dda9/packages/editor/src/lib/app/App.ts#L1479-L1483

The above means that if I call this.updateUserDocumentSettings({ isDarkMode }, true) by itself it'll miss the event entirely. An example, if I wanted made a method that synced settings across tabs. I would likely just call updateUserDocumentSettings updating all the settings in one shot. With these changes I'd have to write a separate event handler for each way I want to use the core data.

In my experience leads to missed data / events. If you were using this for analytics for example it could be months of incorrect data / decisions before the team discover the issue, I've personally seen this may times.

I'd prefer to derive the data from the store. In the original commits I was using the onAfterChange for this but I also think we could just use the computed from signia (CC @ds300).

Either way I think we want a single change that produces side effects (react-ey) rather than putting the side-effects on various functions around the codebase.

steveruizok commented 1 year ago

My main concern there is that there are some intents, such as duplicating a page, that can't be seen by examining the changes alone. There are also changes that, if observed, could have be triggered by many changes: for example "create page" being caused by duplicating a page, creating a new page, moving shapes to a new page, undoing a delete page, redoing a create page, etc. There's also the case where actions from other users might get reported as having come from the user, for example when a page is added to the store by a collaborator. 😮‍💨

At this point, I think it would be better to catch and report changes based on the intent we want to capture, rather than specifically what happened in the store. tldraw really isn't an event-driven application, so if a developer really wanted to know every time a shape appeared on the canvas (for any reason), I'd rather point them to the store.listen method. Maybe we can prefix the events here to be more explicit: user-create-shapes, user-duplicate-page, etc?

I'd earlier considered moving certain events (like toggling dark mode) out to the UI actions in useActions, so that the event is emitted when the action runs rather than when the API method runs. This wasn't a full solution, however, as we'd also need to emit changes like create-shapes from the editor alone.

orangemug commented 1 year ago

I guess here are my concerns.

If I'm listening to "page:new" event. I expect it to trigger when a new page get added, no mater how it gets added.

Because that's the expected behaviour of attaching an event listener to an app.

I think "duplicate page" is the exception (that's a little odd) but most things we can derive from the store, keeping stuff derived from the store where we can will limit the bugs in the application and make development easier.

tldraw really isn't an event-driven application

Yes this was the intention in the original branch, where we had a userTrack(...) call or something like that.

It looks like you're trying to add a general purpose event listener, where you seem to not really want one (if I understand correctly). If we want to add events, I think we should consider that and make that a possible approach to develop an app against. If we don't want an events API, then we shouldn't add it at all.

steveruizok commented 1 year ago

Ok, I think this is good discussion. Looking back at the events, there seem to be a few different categories:

Changes

Things that happen as the result of some change to the store. For example, the currentPageId may change for many reasons, such as when deleting the current page, duplicating a page, moving shapes to a page, or having someone else delete your current page. Likewise, shape records may be created or removed for many reasons.

These are all events that could be seen directly from the store, and so could be emitted automatically via reactors or via the onAfterCreate / onAfterDelete / onAfterUpdate calls as @orangemug did here.

Editor Lifecycle

Events that refer to moments within the editor's life cycle:

Actions

Higher-level method calls, such as App.zoomIn or App.duplicateShapes, which make lower changes to some records. An example being App.zoomIn, which makes a change to the camera record, or App.duplicateShapes, which calls other methods (App.createShapes). These may be trigged by calling the methods or by interacting with our UI (which also calls the methods).

Pure-ui Events

Events that really only

I think the move here would be to skip the change events (or rather, to subscribe to them using reactors). I'd like to have / share analytics for when users use tools to create shapes, however we may be able to derive shape use by looking at other data.

steveruizok commented 1 year ago

I'll merge in some changes when I get to the office that will bring this PR in line with my comment above.

orangemug commented 1 year ago

tldraw really isn't an event-driven application

...If we want to add events, I think we should consider that and make that a possible approach to develop an app against.

Sorry @steveruizok I guess what I meant by this was our intention here is to get analytics into the app. We seem to have diverged into adding general purpose events.

I think that's a different task, and would probably require a little more research. So I think to do that well we should look at existing libraries like maplibre-gl-js events and leaflet and other also other drawing apps. I think a good API here is something that feels natural, something you can almost guess because you've used similar libraries before with a camera / drawing tools etc...

If we just use a callback for now we can mark the whole API as @internal and leave this decision for another day.

Otherwise we're going to have to mark particular events from the event emitter as @public/@internal which seems to add more confusion to our API.

We could also research and general purpose events API. But I'd like to have a reason for adding that and some aims/usecases first.

steveruizok commented 1 year ago

Ok, I've lifted the non-critical events out of the app and up into the UI layer in a manner similar to your original branch. We won't capture events on method calls (e.g. app.alignShapes) but we will catch them when we use our actions and tools. I think should give us enough to work with without adding much to our API.

orangemug commented 1 year ago

I'm not sure where this PR is at the moment. We seem to be rewriting the API, @SomeHats is the API person by the sounds of things

A few comments from me: If we are writing the API setIsFocusMode(...) should probably be setFocusMode(...) this is comment from an earlier PR comment was refering to the norms of [isFoo, setFoo] = useState(...) usage.

It's more normal to see app.setFocusMode(boolean) in the wild. Don't think I've ever seen something setIsSometing in an API (maybe others have).

I'd probably just design the API we want, and do that in a single PR. Else we'll have to major update way too often for library consumers. I think the main API should rarely change, it's also probably discussing and designing in a team given together we'll probably have a good knowledge of existing APIs and how we can fit into the norm (RFC?)

orangemug commented 1 year ago

Also "release note" (in PR description) doesn't match the changes we should be outlining the API changes in this section.