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: Add line chart and paste dialog selection #2670

Merged
merged 39 commits into from
Dec 27, 2020
Merged

feat: Add line chart and paste dialog selection #2670

merged 39 commits into from
Dec 27, 2020

Conversation

lipis
Copy link
Member

@lipis lipis commented Dec 26, 2020

So it looks like the world is not ready for the fast paste approach. Here is a more deterministic solution by @petehunt's idea.

Copy the following data and experiment in the preview.

N,Fibonacci
F1,1
F2,1
F3,2
F4,3
F5,5
F6,8
F7,13

In this PR

We are adding a third step, but to be honest, in real life we are not a chart replacement and people won't be pasting charts that often, so this is not really annoying as it shows already what is going to be inserted and can keyboard could be used for power users.

Screenshot 2020-12-26 at 6 15 07 PM


  • Paste chart modal
  • Show previews of charts
  • Insert charts in the middle of the screen (@dwelle)
  • Use Arrow Keys Tab key to choose chart (@dwelle)
  • Press Enter or Space to insert (@dwelle)
  • Remember the last choice and have it default next time (blue border) if user press Enter
  • UI tweaks (colors, size, etc) (@j-f1)
  • Mobile view (@j-f1)

Possible improvements as we go (if any)

  • Show Remember my choice and don't show this popup again
    • Show this popup with a special shortcut that will appear in Shortcuts dialog.
  • Special shortcuts for pasting directly Bar or Line charts
    • We can run out of smart shortcuts here
  • Special submenu in the Context menu for pasting special
    • Downside it's too hidden and we don't have access to Clipboard in many browsers without permission.

…charts

* 'master' of github.com:excalidraw/excalidraw:
  feat: tweak editing behavior (#2668)
  chore: update Sentry (#2669)
  chore: New Crowdin updates (#2620)
  chore: Remove unused cursorX, cursorY from AppState (#2665)
…ne-chart

* 'master' of github.com:excalidraw/excalidraw:
  feat: tweak editing behavior (#2668)
  chore: update Sentry (#2669)
  chore: New Crowdin updates (#2620)
  chore: Remove unused cursorX, cursorY from AppState (#2665)
…o new-line-chart

* 'new-line-chart' of github.com:excalidraw/excalidraw:
  Update constants.ts
@vercel
Copy link

vercel bot commented Dec 26, 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/hq7x54jih
✅ Preview: https://excalidraw-git-paste-charts.excalidraw.vercel.app

@lipis lipis marked this pull request as draft December 26, 2020 03:13
src/types.ts Outdated Show resolved Hide resolved
@dwelle
Copy link
Member

dwelle commented Dec 26, 2020

While it adds friction, I reckon you also won't be pasting charts all the time, so it's kinda ok? That said, I noticed showPasteChartDialog which I guess will allow you to [x] remember my choice. But how do you then revert this choice?

@lipis
Copy link
Member Author

lipis commented Dec 26, 2020

But how do you then revert this choice?

By fast pasting!! 🤣 (Just kidding.. I updated the description)

@dwelle
Copy link
Member

dwelle commented Dec 26, 2020

Reading the updated description, I'd start with what you suggest: simply highlight what was selected previously. Later we can iterate on the remember my choice. I wouldn't add chart-specific shortcuts. As for the contextmenu, that can be implemented as an alternative either way (in the future).

@dwelle
Copy link
Member

dwelle commented Dec 26, 2020

I think this is good. Let's implement it and ship it! 🚀

@dwelle
Copy link
Member

dwelle commented Dec 27, 2020

I've moved currentChartType outside of charts because it doesn't really belong there. We should maybe also rename appState.charts to appState.chartsDialog again?

I've also improved the type.

@lipis
Copy link
Member Author

lipis commented Dec 27, 2020

Yeah.. go for appState.chartsDialog appState.pasteDialog (discussed)

…charts

* 'master' of github.com:excalidraw/excalidraw:
  chore(deps-dev): bump firebase-tools from 9.0.1 to 9.1.0 (#2676)
  style: media query for hiding shortcuts for mobile view (#2667)
@lipis lipis changed the base branch from new-line-chart to master December 27, 2020 13:39
@lipis lipis changed the title feat: Show modal on paste chart feat: Add line chart and paste dialog selection Dec 27, 2020
Copy link
Member

@j-f1 j-f1 left a comment

Choose a reason for hiding this comment

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

LGTM except for the issue with pasting on mobile, although we can address that in a future PR

@lipis

This comment has been minimized.

Copy link
Member

@dai-shi dai-shi left a comment

Choose a reason for hiding this comment

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

I didn't check the code, but UI-wise it's good to go.

Copy link
Member

@ad1992 ad1992 left a comment

Choose a reason for hiding this comment

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

Checked the UI, LGTM 👍

@dwelle
Copy link
Member

dwelle commented Dec 27, 2020

Looks good. Merging. 🎉

@lipis lipis merged commit 022f349 into master Dec 27, 2020
@lipis lipis deleted the paste-charts branch December 27, 2020 16:26
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

7 participants