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

Library MVP #1787

Merged
merged 15 commits into from Jul 10, 2020
Merged

Library MVP #1787

merged 15 commits into from Jul 10, 2020

Conversation

petehunt
Copy link
Contributor

MVP implementation of a shape library.

Demo video here: https://drive.google.com/file/d/1P75OmTDKuSlJ8K1_15go9-LswGgGjF5E/view?usp=sharing

In the future we'll want to provide ways for people to import libraries (perhaps by dragging and dropping a library file?). I can add a basic version of this in a follow up PR if we are in agreement that this user experience is good enough.

@vercel
Copy link

vercel bot commented Jun 20, 2020

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/3be3ldi3p
✅ Preview: https://excalidraw-git-fork-petehunt-palette.excalidraw.vercel.app

@dwelle
Copy link
Member

dwelle commented Jun 20, 2020

Nice.

A few notes:

  • Clearing canvas shouldn't clear library.
  • Overflows screen without scrollbar.
  • Doesn't show on mobile (EDIT: checking code, this seems intentional).
  • It may not be obvious that you should drag & drop from library to canvas. Maybe it should also work on click, where it would paste the element at the center of viewport. And/or display a hint that you can drag & drop.
  • It took me a while that the library is hidden under contextmenu. Maybe for MVP this is fine, but we should think about it exposing it from the start.
  • Maybe: prevent adding duplicate library elements (by looping through existing and comparing their props, excluding version, versionNonce, id...).

@petehunt
Copy link
Contributor Author

petehunt commented Jun 20, 2020

Overflows screen without scrollbar.

how should we handle this? can i change the mobile breakpoint?

@dwelle
Copy link
Member

dwelle commented Jun 20, 2020

how should we handle this? can i change the mobile breakpoint?

How does mobile breakpoint tie into this?

@petehunt
Copy link
Contributor Author

I think the solution (unless I'm misunderstanding the issue) is to switch to the mobile view when the viewport gets too narrow to show the full shape switcher. Right now this is controlled by the mobile breakpoint media query (set to 640px right now). Do I understand the issue correctly?

@dwelle
Copy link
Member

dwelle commented Jun 20, 2020

Oh, I meant that when the library gets too large (too many items), there's no vertical scrollbar and instead it overflows off screen.


What you're talking about is a problem in itself, but it's the opposite actually. Small mobile screens won't fit all the tools on one line. That said, iphone-5 screen sizes won't fit text tool even now (my Android fits text tool just so-so. The library icon won't fit at all) --- I'd put the tool to the bottom toolbar on mobile (but there is space for it only when nothing is selected, on my phone anyway).

image image

@vjeux
Copy link
Contributor

vjeux commented Jun 23, 2020

I really like the prototype. Some thoughts on the UI:

  • It's very weird that the button doesn't show up until you do some hidden command. I would always show it.
  • Adding something to the library is not very intuitive right now. The way I thought about was to click on the library icon and have a + button there which saves whatever you have currently selected.
  • It's not 100% clear that you can drag and drop to display. I would also enable click which puts the shape in the center of the screen.
  • Would be nice to have key bindings "9" and then "1", "2"... so you can quickly add elements to the screen.
  • You normalize the size of all the elements in the preview. I feel like we should keep it the same size as they would be displayed and have a max-dimension so it doesn't display too big (with some kind of visible hint that it overflows).

I have a lot of questions around how do you share those, how do they work with live collaboration... But they can be addressed later.

@petehunt
Copy link
Contributor Author

will fix all these issues, plus will enable on mobile and store the library in a separate localstorage key that doesn't get cleared when the canvas is cleared

@vjeux vjeux mentioned this pull request Jun 29, 2020
11 tasks
@bduffany
Copy link

Nice solution! Looks like you beat me to it. I created a similar feature here: #1834

Maybe we can come up with a design that combines the best of both approaches?

@petehunt
Copy link
Contributor Author

petehunt commented Jul 2, 2020

hey @bduffany -- i was planning on revisiting this PR on Sunday, but it's been hard for me to find time lately to work on this sort of thing. i'm totally down to have you finish it off if you'd like! just please let me know in the next day or two so i don't end up doing it this weekend :)

if you're going to take it over, i would consider pushing @dwelle's good suggestion of preventing duplicates in the library to another diff -- i found it to be a fairly involved change as you need to take into account some nondeterminism with floating point math and i think it's reasonable to do it later (@dwelle let me know if that's ok!)

additionally, i would suggest keeping it in appState for now and landing the localStorage piece in a separate diff -- this PR is already getting pretty big and it'd make code review easier.

again, just please let me know in the next day or two whether you want to take this over, otherwise i'll try to finish it up this weekend. thanks!

@dwelle
Copy link
Member

dwelle commented Jul 3, 2020

if you're going to take it over, i would consider pushing @dwelle's good suggestion of preventing duplicates in the library to another diff

This is fine.

additionally, i would suggest keeping it in appState for now and landing the localStorage piece in a separate diff -- this PR is already getting pretty big and it'd make code review easier.

But I'd argue we should do it in this PR, otherwise we'd need to write a migration which isn't worth it IMO. If you won't find time, I can do that.

@petehunt
Copy link
Contributor Author

petehunt commented Jul 6, 2020

OK, I think this PR is reviewable. I think I did everything except:

  • Fix the toolbar to work well on mobile. This was already an issue and would prefer to do it separately
  • Support hotkeyed library items. I think this can be added later, and since it's going to be the first time we overload hotkeys depending on whether a menu is open I think it's worth thinking about longer and doing a proper refactor.
  • Didn't do size normalization, can be done later / ran out of time :)

Please double check the library loading logic -- not sure if I got all the places where app state is reloaded. And let me know what you think about the new adding experience.

Always show library

Show library on mobile

click to add

Put a max height + scroll on the library menu

More discoverable 'add to library' behavior

9 to open library

Save and load library from local storage

Make library openable on mobile
@dwelle
Copy link
Member

dwelle commented Jul 8, 2020

We should either keep library out of appState, or rewrite how it's being loaded. Either way, loading it in getDefaultAppState() isn't good. That function should be pure(ish), and not tied to state. It also wouldn't work on node that way, and we'd need to make it async when we migrate to IDB.

Comment on lines +351 to +356
exportToBackend(elements, {
...appState,
viewBackgroundColor: exportBackground
? appState.viewBackgroundColor
: getDefaultAppState().viewBackgroundColor,
});
Copy link
Member

Choose a reason for hiding this comment

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

unrelated

Copy link
Contributor Author

Choose a reason for hiding this comment

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

thank you!

@@ -316,6 +318,11 @@ class App extends React.Component<ExcalidrawProps, AppState> {
this.setState(
(state) => ({
...actionResult.appState,
// use prev library to guard against clearing it when restoring etc.
Copy link
Contributor Author

Choose a reason for hiding this comment

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

i think this breaks the right click "add to library" menu option

Copy link

@bduffany bduffany Jul 9, 2020

Choose a reason for hiding this comment

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

Just curious, why use appState for storing the drawing library? Seems like the library can be thought of as an entity separate from the app which has its own state. Multiple different tabs (each with their own app state) may be working on different drawings yet accessing the same library so I'm not sure whether modeling the library as part of the app state makes sense in that framing.

Copy link

Choose a reason for hiding this comment

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

If you are worried about getting updates when localStorage is updated, you can use the subscribe pattern like so:
https://github.com/excalidraw/excalidraw/pull/1834/files#diff-ae70f4fb5856a0e21ead44ca69f0cd7c

The only thing that's missing from that file is to listen to storage events from the window and invoke notify on each storage event (i.e. localStorage updates from other tabs)

Copy link
Member

Choose a reason for hiding this comment

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

I agree that we should factor it outside of appState, but let's leave it for a following PR. I'll take a look at the addToLibrary action (facepalm on the comment saying we currently don't update library via syncActionResult).

Copy link
Member

Choose a reason for hiding this comment

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

Now that I look at it, we may wanna migrate to that workflow right now, so as not pile more on hacks still.

@dwelle
Copy link
Member

dwelle commented Jul 9, 2020

Pushed the factoring out of appState.

Only downside I'm seeing is hooking it into history later, so that we can undo the action. But let's cross that bridge when we come to it.

Before merge, I wanna add a caching layer in the evening.

@dwelle
Copy link
Member

dwelle commented Jul 9, 2020

Pushed 3 commits.

  • made loadLibrary() async in anticipation of migration to IDB
  • since loading from IDB can take 100ms+ in edge cases, I added a loading state which shows if the library doesn't load under 100ms
  • added a cache layer so that subsequent calls to loadLibrary() resolve immediately

@bduffany
Copy link

Woohoo! This is huge :)

@dwelle
Copy link
Member

dwelle commented Jul 10, 2020

@bduffany Feel free to make followup PRs to improve this. I especially liked your anim when you added the shape to library.

I'll create issues what needs to be done when I have time.

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

4 participants