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

fix(tldraw): enable initial currentPageId prop #857

Closed

Conversation

with-heart
Copy link
Contributor

@with-heart with-heart commented Jul 28, 2022

This PR fixes tldraw/tldraw-v1#187

Opening this as a draft because I'm really not sure what the best approach would be here.

tldraw/tldraw-v1#187 occurs because TldrawApp initializes its state from TldrawApp.defaultState which doesn't reflect the reality of our intentions if we're passing custom document/currentPageId props to Tldraw:

Us: Hey Tldraw, please render page1 from our customDocument
Tldraw: Sure thing! Rendering page from TldrawApp.defaultDocument


For an initial attempt, I added a third initialState argument to TldrawApp#constructor that defaults to TldrawApp.defaultState. I then added a new object in the initializers in Tldraw using TldrawApp.defaultState, overriding specifically the document and appState.currentPageId keys since those are the only parts of initial state relevant to Tldraw.

This allows us to set the custom initial state we need without any breaking changes. The downside is that the initialization piece is gross af imo. Consumers probably shouldn't need to spread TldrawApp.defaultState like that, but I couldn't think of a better way to handle it.

I had considered making initialState optional and deep merging it into TldrawApp.defaultState when provided, but since we need to replace defaultState.document with props.document if it exists, deep merging isn't an option here. We could make the new argument an object of type { document: TDDocument; currentPageId: string } and do the spreading ourselves inside TldrawApp, but that feels too specific to only the Tldraw use case.

Would love some additional thoughts or context to help shape this into a better solution!

@vercel
Copy link

vercel bot commented Jul 28, 2022

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

Name Status Preview Updated
core ✅ Ready (Inspect) Visit Preview Sep 2, 2022 at 2:34PM (UTC)
tldraw ✅ Ready (Inspect) Visit Preview Sep 2, 2022 at 2:34PM (UTC)

@steveruizok
Copy link
Collaborator

Wow, I spent some time working on this today and it really is a thorny issue. See what you can come up with, I'll also try some things out here.

@with-heart with-heart force-pushed the fix-tldraw-currentPageId-prop branch from 08ca59b to b74d081 Compare August 1, 2022 01:44
@with-heart
Copy link
Contributor Author

with-heart commented Aug 1, 2022

@steveruizok I spent more time on this today than I'd care to admit 😆 but I think I've come up with something that feels pretty good

Still need to actually play around with the changes in the example app just to make sure it's working the way I think it will, but gonna call it a day so will hopefully get a chance to do that tomorrow

@with-heart
Copy link
Contributor Author

Played around with it, seems to be working!

@with-heart with-heart marked this pull request as ready for review August 1, 2022 15:24
@with-heart with-heart marked this pull request as draft August 2, 2022 13:15
@with-heart
Copy link
Contributor Author

Actually I wasn't thorough enough with testing.

It works fine if document has a single page or currentPageId corresponds to the first page in document.pages.

If you have multiple pages and currentPageId is set to a non-first page, it initially renders the correct page but resets back to displaying the first page after onReady calls this.loadDocument:

const state = {
...TldrawApp.defaultState,
settings: {
...this.state.settings,
},
document,
appState: {
...TldrawApp.defaultState.appState,
...this.state.appState,
currentPageId: Object.keys(document.pages)[0],
disableAssets: this.disableAssets,
},
}

It works if I don't reset currentPageId there, but then resetting the document won't work as expected. Need to think about how to handle that situation

@with-heart
Copy link
Contributor Author

with-heart commented Aug 3, 2022

In the current state, we can at least pass an onMount callback that calls app.changePage(currentPageId) to set the correct page after the document loads. Maybe that's enough for a temporary solution?

Shared more detailed thoughts in tldraw/tldraw-v1#187

@steveruizok
Copy link
Collaborator

It might need to be for now. Quite frustrating due to the need to wait for the async indexeddb load.

@judicaelandria
Copy link
Contributor

In the current state, we can at least pass an onMount callback that calls app.changePage(currentPageId) to set the correct page after the document loads. Maybe that's enough for a temporary solution?

Shared more detailed thoughts in tldraw/tldraw-v1#187

I was thinking about fixing this too, quite frustrating when it always comes back to the first page. I think you can do that IMO

@with-heart
Copy link
Contributor Author

Sorry this has been languishing! Will try to get it updated and ready for merge later today. Gonna go with the onMount callback to make things work for now

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.

[bug] currentPageId prop has no effect
3 participants