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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat: add view mode in Excalidraw #2840

Merged
merged 30 commits into from Feb 1, 2021
Merged

feat: add view mode in Excalidraw #2840

merged 30 commits into from Feb 1, 2021

Conversation

ad1992
Copy link
Member

@ad1992 ad1992 commented Jan 22, 2021

Fixes #2830 Fixes #2841

This is an initial version of how view mode would look like 馃憞 Please share your views.

Excalidraw (6)

For package 馃憞
You can check it here

Version 1 (this PR)

  • Add the action to the context menu: Read-only (just like Zen mode)
  • Hide the shapes toolbar
  • Hide the properties toolbar
  • Only show save, save as, and export options in the top left toolbar
  • Disallow modifying / deleting elements.
  • Disallow pasting elements or styles.
  • Make the context menu contextual and only show minimal options applicable for read-only mode
    • Copy to clipboard as png
    • Copy to clipboard as SVG
    • show stats for Nerds
    • Read-only mode
  • Update mobile view
  • Add a shortcut alt+r for view mode
  • Allow zooming, panning and touch gestures on canvas.
  • Add viewModeEnabled prop so the host can also use it. We don't render view mode in context menu when this prop is true so host can control it.
  • show collaborators.
  • zooming is not working in mobile, fix it
  • testing
  • Update readme and changelog

Version 2 (next PR)

  • Allow selection of individual elements.
  • Make keyboard shortcuts contextual.

what else?

cc @dwelle

@vercel
Copy link

vercel bot commented Jan 22, 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/h4sbatay9
鉁 Preview: https://excalidraw-git-aakansha-ronly.excalidraw.vercel.app

@ad1992 ad1992 changed the title feat: add readonly prop which renders readonly Excalidraw mode feat: add readonly prop which renders Excalidraw in readonly mode Jan 22, 2021
@dwelle
Copy link
Member

dwelle commented Jan 22, 2021

Looks promising. We'll need to factor out logic relating to canvas movements (panning, zoom...) and such.

Dunno what's our stance on exporting.

Ad help dialog: maybe. Or we can leave it for later PR.

@ad1992 ad1992 changed the title feat: add readonly prop which renders Excalidraw in readonly mode feat: add readonly prop and an option in context menu which renders Excalidraw in readonly mode Jan 23, 2021
@ad1992 ad1992 changed the title feat: add readonly prop and an option in context menu which renders Excalidraw in readonly mode feat: add readonly mode in Excalidraw and support for host. Jan 24, 2021
@dwelle
Copy link
Member

dwelle commented Jan 24, 2021

  • panning doesn't work on desktop
  • rn you can right-click an element to select it (which you then cannot deselect)

I have only now read #2841 so I'll discuss some more points over there

Co-authored-by: Lipis <lipiridis@gmail.com>
@dwelle
Copy link
Member

dwelle commented Feb 1, 2021

I think the cursor in view mode should act as a drag tool, as does in Figma/Miro etc. and we shouldn't deviate from that standard.

I know we're still not splitting the modes into presentation & read at this point, but this I think should be in both modes.

In terms of how selection will work in that mode: I like what Miro does, which is dragging always drags, and single-clicking on an element selects (plus changes cursor from hand to normal arrow). If we want to leave this part for a next PR, that's fine, but I'd at least start with dragging tool across the board.

@dwelle
Copy link
Member

dwelle commented Feb 1, 2021

Also, if we're gonna persist the view mode into LS across refreshed, I'd add the exit view mode button otherwise people will get locked out over time (or if someone new accesses the workstation where someone left view mode on).

@ad1992
Copy link
Member Author

ad1992 commented Feb 1, 2021

Also, if we're gonna persist the view mode into LS across refreshed, I'd add the exit view mode button otherwise people will get locked out over time (or if someone new accesses the workstation where someone left view mode on).

I think we shouldn't store it in local storage as this doesn't seem like a mode which users would want to persist. For example, if you check in google docs (change to view mode & it is lost on refresh) and when we use this mode in a collaboration that time mode will always be persisted if the view-only link is shared.

Thoughts?

@lipis
Copy link
Member

lipis commented Feb 1, 2021

馃悜 it

@ad1992
Copy link
Member Author

ad1992 commented Feb 1, 2021

I think the cursor in view mode should act as a drag tool, as does in Figma/Miro etc. and we shouldn't deviate from that standard.

I know we're still not splitting the modes into presentation & read at this point, but this I think should be in both modes.

In terms of how selection will work in that mode: I like what Miro does, which is dragging always drags, and single-clicking on an element selects (plus changes cursor from hand to normal arrow). If we want to leave this part for the next PR, that's fine, but I'd at least start with dragging the tool across the board.

Do you mean we should show a hand cursor instead of the disabled cursor in view mode? And by dragging tool you mean something like below 馃憞 But wouldn't this mean we are allowing the user to update the whole drawing position in view mode ?

Kanban Framework, Online Whiteboard for Visual Collaboration

@dwelle
Copy link
Member

dwelle commented Feb 1, 2021

but wouldn't this mean we are allowing the user to update the whole drawing position in view mode ?

I'm talking about panning. That modifies local client's scroll position. Same as what is shown in your Miro GIF above.

@ad1992
Copy link
Member Author

ad1992 commented Feb 1, 2021

@dwelle @lipis I have allowed panning when no elements selected. If this looks fine, let's ship this and we can improve it in the next PR?

@lipis
Copy link
Member

lipis commented Feb 1, 2021

I'm down.. with follow up improvements..!

@dwelle
Copy link
Member

dwelle commented Feb 1, 2021

Let's at least fix this: the panning only works every 2nd drag you do :)

@ad1992
Copy link
Member Author

ad1992 commented Feb 1, 2021

My approach of checking selectedElementIds might not be right when allowing selection over panning. I am looking into it but might need some more time to debug this. The selection also requires a double click sometimes.

Do we want to disable selection and allow only panning for first version? Then resetting the selectedElementIds will be needed when changing to view mode.

@lipis
Copy link
Member

lipis commented Feb 1, 2021

There is no wrong way for the first iterration.. anything we don't like or want to improve can file as issues..

@dwelle
Copy link
Member

dwelle commented Feb 1, 2021

Do we want to disable selection and allow only panning for first version? Then resetting the selectedElementIds will be needed when changing to view mode.

I think this is a good idea. Better than to ship it in a half broken state. (Also, selecting is an edge case. Panning is more important in view mode.)

@ad1992
Copy link
Member Author

ad1992 commented Feb 1, 2021

There is no wrong way for the first iterration.. anything we don't like or want to improve can file as issues..

Sure sounds good 馃憤 . If this bug is not a blocker let's ship it 馃殌 .

@dwelle what do you say?

@ad1992
Copy link
Member Author

ad1992 commented Feb 1, 2021

Do we want to disable selection and allow only panning for first version? Then resetting the selectedElementIds will be needed when changing to view mode.

I think this is a good idea. Better than to ship it in a half broken state. (Also, selecting is an edge case. Panning is more important in view mode.)

Do we want to disable selection and allow only panning for first version? Then resetting the selectedElementIds will be needed when changing to view mode.

I think this is a good idea. Better than to ship it in a half broken state. (Also, selecting is an edge case. Panning is more important in view mode.)

missed this comment. should be fine now 馃憤

@dwelle
Copy link
Member

dwelle commented Feb 1, 2021

Thanks. Shipping! 馃帀

@dwelle dwelle merged commit 675da16 into master Feb 1, 2021
@dwelle dwelle deleted the aakansha-ronly branch February 1, 2021 20:56
lipis added a commit that referenced this pull request Feb 4, 2021
* 'master' of github.com:excalidraw/excalidraw: (58 commits)
  chore: Update translations from Crowdin (#2906)
  fix: toolbar unnecessarily eats too much width (#2924)
  fix: mistakenly hardcoding scale (#2925)
  fix: text editor not visible in dark mode (#2920)
  feat: support supplying custom scale when exporting canvas (#2904)
  fix: incorrect z-index of text editor (#2914)
  feat: Show version in the stats dialog (#2908)
  fix: make scrollbars draggable when offsets are set (#2916)
  chore: Run actions on pull requests as well (#2917)
  feat: Add idle detection to collaboration feature (#2877)
  fix: pointer-events being disabled on free-draw (#2912)
  feat: don't store to LS during collab (#2909)
  chore: Update translations from Crowdin (#2898)
  fix: track zenmode and grid mode usage (#2900)
  refactor: Use the latest vercel configuration instead of now (#2893)
  chore: Update translations from Crowdin (#2894)
  Update i18n.ts
  Update locales-coverage-description.js
  feat: add view mode in Excalidraw (#2840)
  chore(deps): bump @sentry/integrations from 6.0.1 to 6.0.3 (#2889)
  ...
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.

Add read only mode as an option Add host support to render Excalidraw in view mode
3 participants