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

feat: Make library local to given excalidraw instance and allow consumer to control it #3451

Merged
merged 14 commits into from Apr 21, 2021

Conversation

ad1992
Copy link
Member

@ad1992 ad1992 commented Apr 17, 2021

An attempt to attach the library to each excalidraw instance so each instance has its own library when multiple excalidraw rendered.

fixes #3400

Try here

  • Add props

    1. onLibraryChange which will get triggered on every library change.
    2. initialData.libraryItems with which host can load excalidraw with existing library items.
    3. Add unique id to excalidraw component which will be available to host via ref.
  • Testing

  • Update changelog & readme

@vercel
Copy link

vercel bot commented Apr 17, 2021

This pull request is being automatically deployed with Vercel (learn more).
To see the status of your deployment, click below or on the icon next to each commit.

🔍 Inspect: https://vercel.com/excalidraw/excalidraw/CHK9A8qMkMfk6rLEogc7fyiH8jyN
✅ Preview: https://excalidraw-git-aakansha-library-excalidraw.vercel.app

@dwelle
Copy link
Member

dwelle commented Apr 17, 2021

I'm thinking, is it actually what you want when you add/remove an item to a library in one instance and it's not reflected in the other? Is the plan for it to be configurable?

@ad1992
Copy link
Member Author

ad1992 commented Apr 17, 2021

I'm thinking, is it actually what you want when you add/remove an item to a library in one instance and it's not reflected in the other?

rn the library is shared among all the instances so add/removal or any action in the library will get refected in other instances which shouldn't happen, this is an attempt to fix that & make sure each exaclidraw component has its own library.

Is the plan for it to be configurable?

rn, the data is saved to local storage for the library, I am thinking this should be the responsibility of the host and not excalidraw. WDYT?

@dwelle
Copy link
Member

dwelle commented Apr 17, 2021

rn the library is shared among all the instances so add/removal or any action in the library will get refected in other instances which shouldn't happen

Shouldn't it happen though? Maybe it should, but I can't think of any use cases — but then I'm not sure what are the uses cases for multiple components on the same page, so. What do you have in mind?

rn, the data is saved to local storage for the library, I am thinking this should be the responsibility of the host and not excalidraw. WDYT?

True, once we move LS to host then it can decide on its own whether it wants to save the libraries to a single namespace or not.

@ad1992
Copy link
Member Author

ad1992 commented Apr 17, 2021

rn the library is shared among all the instances so add/removal or any action in the library will get refected in other instances which shouldn't happen

Shouldn't it happen though? Maybe it should, but I can't think of any use cases — but then I'm not sure what are the uses cases for multiple components on the same page, so. What do you have in mind?

No, when you have multiple instances of excalidraw on same page modifying library in one of the excel comp will affect the other excal comp which shouldn't happen and should only update the library in the active comp as its attached to excal and not outside excal hence more intuitive as well. What use case do you think it should be treated as shared thing ?
coz I feel when there are multiple excal comp, it should be treated as an entity of each excal comp and not a shared one.

@dwelle
Copy link
Member

dwelle commented Apr 17, 2021

No, when you have multiple instances of excalidraw on same page modifying library in one of the excel comp will affect the other excal comp which shouldn't happen and should only update the library in the active comp as its attached to excal and not outside excal hence more intuitive as well. What use case do you think it should be treated as shared thing ?
coz I feel when there are multiple excal comp, it should be treated as an entity of each excal comp and not a shared one.

Yes, but why? From my perspective the library doesn't belong to any one component. Depending on the implementation, I'd be interested in how you want it to behave in multi-component scenario.

First, what are you gonna key the library by? If a random id generated by each component, that id won't persist across sessions, so each component would have only an ephemeral library state that would reset each time (maybe you're arguing for this use case #3400?).

Even if you make the keys persistent (e.g. by supplying them manually from host), is it useful for one component to have its library be separate from the other? Is it intuitive to add an element into the library in one compo, and then when you open the library in the other it won't be there?

Again, depends on the use case, so I'd be interested if you have one or not (like I said, I don't really have one, so I can only fall back to our use case).

@ad1992
Copy link
Member Author

ad1992 commented Apr 17, 2021

No, when you have multiple instances of excalidraw on same page modifying library in one of the excel comp will affect the other excal comp which shouldn't happen and should only update the library in the active comp as its attached to excal and not outside excal hence more intuitive as well. What use case do you think it should be treated as shared thing ?
coz I feel when there are multiple excal comp, it should be treated as an entity of each excal comp and not a shared one.

Yes, but why? From my perspective the library doesn't belong to any one component. Depending on the implementation, I'd be interested in how you want it to behave in multi-component scenario.

First, what are you gonna key the library by? If a random id generated by each component, that id won't persist across sessions, so each component would have only an ephemeral library state that would reset each time (maybe you're arguing for this use case #3400?).

Even if you make the keys persistent (e.g. by supplying them manually from host), is it useful for one component to have its library be separate from the other? Is it intuitive to add an element into the library in one compo, and then when you open the library in the other it won't be there?

Again, depends on the use case, so I'd be interested if you have one or not (like I said, I don't really have one, so I can only fall back to our use case).

So the way I am looking at it is each excal instance should be independent of the other, so for example there can be a scenario where a user wants to load diff excal component with different library items, since its inside excalidraw comp hence it gives an impression that it should be treated as an entity of excal comp rather than a shared one. As a user i found it confusing when deleting items from one excal instance affected the other.
Again this is a scenario which I am considering, I understand your perspective you are basically seeing it as a collection for drawings common across excal components, I think I am ok with that as well and hold this PR until we face any real time scenario where host will want to treat it separately. For the library key I was gonna get it from host so its persisted as well.

Since I got confused when modifying library in one component so its better we mention in the readme that library will be shared acorss comp so users don't get confused/surprised, and we can revisit this PR when we realize there is a real scenario where host would want to treat it separately.

@dwelle
Copy link
Member

dwelle commented Apr 17, 2021

I think it's ok to move forward with this once we make the library LS controlled by host + we make it easy to update library of components (AFAIK rn you can only do importLibrary which I think is not appropriate for this, but I'd have to review) so that hosts can keep the components in sync if desired (but if this will be the common case it may be too much of unnecessary work pushed to the hosts — OTOH, the case for multiple components is uncommon in itself, so maybe this is fine).

@ad1992
Copy link
Member Author

ad1992 commented Apr 18, 2021

I think it's ok to move forward with this once we make the library LS controlled by host + we make it easy to update library of components (AFAIK rn you can only do importLibrary which I think is not appropriate for this, but I'd have to review) so that hosts can keep the components in sync if desired (but if this will be the common case it may be too much of unnecessary work pushed to the hosts — OTOH, the case for multiple components is uncommon in itself, so maybe this is fine).

Sounds good 👍 , l will decouple the LS library, which might need some API updates as well (Will give a separate PR) and then update this PR.

EDIT will do in the same PR as it would bit inter related to the refactoring done in this PR as well.

I think we need not worry about the use case of allowing syncing the library across comp rn and we can decide it upon feedbacks. Letting the host provide an unique identifier(id) should be enough for now so the host can save it correctly

src/types.ts Outdated Show resolved Hide resolved
src/data/library.ts Outdated Show resolved Hide resolved
@ad1992 ad1992 marked this pull request as ready for review April 20, 2021 15:54
@ad1992 ad1992 requested a review from dwelle April 20, 2021 15:54
@ad1992 ad1992 changed the title feat: dnt share library & attach to the excalidraw instance [WIP] feat: dnt share library & attach to the excalidraw instance Apr 20, 2021
src/locales/en.json Outdated Show resolved Hide resolved
Co-authored-by: David Luzar <luzar.david@gmail.com>
 each excalidraw component and remove csrfToken from library as its not needed
Co-authored-by: David Luzar <luzar.david@gmail.com>
@ad1992 ad1992 changed the title feat: dnt share library & attach to the excalidraw instance feat: Make library local to given excalidraw instance and allow consumer to control it Apr 21, 2021
@ad1992 ad1992 merged commit 37d513a into master Apr 21, 2021
@ad1992 ad1992 deleted the aakansha-library branch April 21, 2021 18:08
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.

Support for loading libraries from initialData param
2 participants