grid support (1st iteration) by dai-shi · Pull Request #1788 · excalidraw/excalidraw

dai-shi · 2020-06-20T09:18:38Z

Fixes #521

Thanks to the refactored resize functions, it was pretty easy to add grids for resizing a single element. (Drawing grids was harder.)

TODOs:

enable/disable shortcut (disabled by default)
resize a single element
resize multiple elements
drag a single elements
drag multiple elements
create new elements (rectangles, diamonds, ellipses)
create new elements (two-point lines)

For follow-up PRs: see #1810

context menu to toggle and customize grid size
edit multi-point lines
create new elements (multi-point lines)
create new elements (texts)

vercel · 2020-06-20T09:18:42Z

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/6op6v08x7
✅ Preview: https://excalidraw-git-fork-dai-shi-feat-grid-support.excalidraw.vercel.app

dwelle · 2020-06-20T09:22:27Z

Cool. I thought it's gonna be much harder than this!

we'll want to expose it under a shortcut (Cmd/Ctrl+' is the standard), not as a default
moving elements should move on the grid, too

dai-shi · 2020-06-20T09:26:25Z

And, creating new elements too.

dai-shi · 2020-06-20T23:30:42Z

dai-shi · 2020-06-20T23:31:50Z

src/components/App.tsx

+          } else {
+            dragOffsetXY = getDragOffsetXY([hitElement], x, y);
+          }
+        }


Not 100% sure if this covers all the cases, especially with grouping.

dai-shi · 2020-06-21T02:40:23Z

src/math.ts

  return false;
 };
+
+export const pointOnGrids = (


can suggest a better name?

dai-shi · 2020-06-21T06:46:14Z

I would like to have a context menu to enable/disable grid and configure the grid size (10, 20, 40), but not sure how to add context sub menus.

pshihn · 2020-06-21T07:25:11Z

src/renderer/renderScene.ts

+    context.stroke();
+  }
+  context.strokeStyle = origStrokeStyle;
+};


you can increase the grid render performance by doing a single stroke. Fewer drawing operation the better, every time. (on large screen goes from 0.25ms to 0.05ms) May not be noticeable but every ns counts when drawing :)

context.beginPath(); for (let x = offsetX; x < offsetX + width + gridSize; x += gridSize) { context.moveTo(x, offsetY - gridSize); context.lineTo(x, offsetY + height + gridSize); } for (let y = offsetY; y < offsetY + height + gridSize; y += gridSize) { context.moveTo(offsetX - gridSize, y); context.lineTo(offsetX + width + gridSize, y); } context.stroke();

Btw, since grid never changes per se, we should consider some way of not having to render it every time.

Thanks @pshihn ! Fixed in f4da892.

Btw, since grid never changes per se, we should consider some way of not having to render it every time.

That's not only for grid, but all shapes.

Maybe I can revisit #780 which I closed because the multiplayer didn't play well with that heuristic. Maybe it can be worked around, dunno.

Another crazy idea is to use webgl, which I've experimented in excalidraw-layers. I'm not suggesting it for the current excalidraw, but it might be interesting in general to explore utilizing webgl for 2D.

dai-shi · 2020-06-21T09:23:56Z

src/keys.ts

  F_KEY_CODE: 70,
  ALT_KEY_CODE: 18,
  Z_KEY_CODE: 90,
+  GRID_KEY_CODE: 222,


Not sure about this. 39 didn't work on macos.

dwelle · 2020-06-21T10:49:58Z

I would like to have a context menu to enable/disable grid and configure the grid size (10, 20, 40), but not sure how to add context sub menus.

We'll want submenus sooner or later, anyway. But maybe stick with single grid size for this PR and add the granular support later.

dwelle · 2020-06-21T10:53:58Z

I love this!

dai-shi · 2020-06-21T12:06:21Z

Would you help with the snapshot tests? Don't know why it fails other than gridSize: null.

dwelle · 2020-06-21T13:08:54Z

There's one failing case that seems like a regression (but I couldn't figure out what exactly that regression is, playing with the preview):

 ● duplicate element on move when ALT is clicked › rectangle

    expect(received).toEqual(expected) // deep equality

    - Expected
    + Received

      Array [
    -   30,
    -   20,
    +   0,
    +   40,
      ]

      89 |
      90 |     // previous element should stay intact
    > 91 |     expect([h.elements[0].x, h.elements[0].y]).toEqual([30, 20]);
         |                                                ^
      92 |     expect([h.elements[1].x, h.elements[1].y]).toEqual([-10, 60]);
      93 |
      94 |     h.elements.forEach((element) => expect(element).toMatchSnapshot());

      at Object.<anonymous> (src/tests/move.test.tsx:91:48)

And some of the snapshots are failing on similar significant changes to x/y as well (not just gridSize: null).

dwelle · 2020-06-21T13:23:15Z

Ok, I'm not sure if this is the exact regression, but here's one: when you drag an element and press Alt mid-way, the non-duplicated element should revert to orig position.

dai-shi · 2020-06-22T22:47:55Z

collaboration mode

I don't know which is better. Notice the background color is not in sync.

Maybe someone on the other end wants to draw on grid, but others don't?

On second thought, I can imagine this would happen. So, my suggestion is to keep it separate for now.

dai-shi · 2020-06-23T12:34:51Z

src/components/App.tsx

+        const [gridX, gridY] = getGridPoint(
+          x,
+          y,
+          this.state.elementType === "draw" ? null : this.state.gridSize,


I don't think this is very good in readability. Feel free to refactor...

It's fine. Will be removed when we add support for all types later, anyway.

dwelle · 2020-06-23T12:56:03Z

Looks great!

One last thing, I'd store gridSize when exporting to file (cleanAppStateForExport()) so it's restored on open.

dwelle · 2020-06-23T13:13:39Z

Ad the shortcut. Can you try if Cmd+' works for on you on Figma? I think they must be using event.key because there, on French keyboard (AZERTY) that shortcut doesn't work (~~the ' is one key to the right of QUERTY physical position, but Figma doesn't accept that physical key on my QUERTY ISO keyboard either, so it must be using event.key~~ that key is actually backtick.).

I'd really love to get my hands on Mac (keyboard) so I can do proper debugging.

dai-shi · 2020-06-23T13:32:34Z

(Never used figma...)

Should we go with this?

diff --git a/src/components/App.tsx b/src/components/App.tsx
index dba59fd..27e18b4 100644
--- a/src/components/App.tsx
+++ b/src/components/App.tsx
@@ -1184,7 +1184,7 @@ class App extends React.Component<any, AppState> {
       this.toggleZenMode();
     }
 
-    if (event[KEYS.CTRL_OR_CMD] && event.keyCode === KEYS.GRID_KEY_CODE) {
+    if (event[KEYS.CTRL_OR_CMD] && event.key === KEYS.GRID_KEY) {
       this.toggleGridMode();
     }
 
diff --git a/src/keys.ts b/src/keys.ts
index 10e82bc..d31d0d5 100644
--- a/src/keys.ts
+++ b/src/keys.ts
@@ -16,7 +16,7 @@ export const KEYS = {
   F_KEY_CODE: 70,
   ALT_KEY_CODE: 18,
   Z_KEY_CODE: 90,
-  GRID_KEY_CODE: 222,
+  GRID_KEY: "'",
   G_KEY_CODE: 71,
 } as const;

dwelle · 2020-06-23T13:39:13Z

I dunno TBH. Per discussion on chat, if we go with event.key for this one, it'll take layout into account (which is generally good), but won't work for AZERTY layout (and maybe others) at all :/, whereas what you used previously will work on all layouts, but e.g. on AZERTY the labeling won't be useful for users at all (the physical key will be ù on AZERTY).

What others do:

Figma likely uses event.key, so on AZERTY doesn't work at all.
https://www.photopea.com/ (PS clone) uses event.keyCode so works everywhere, but labeling is off
Affinity (Desktop PS Clone) --- not clear what they do. On AZERTY, it doesn't work at all (even Ctrl+4 which is Ctrl+' for AZERTY).

So probably leave it as you had it previously?

dai-shi · 2020-06-23T13:45:45Z

OK, as there doesn't seem to be a perfect solution, let's leave it.

dwelle · 2020-06-23T14:01:14Z

Actually, keyCode doesn't work on AZERTY at all either, I must have tested it wrong. It seems the ù uses a different keyCode altogether. Anyway, let's leave it for now.

dwelle · 2020-06-23T14:03:34Z

That said, some users wouldn't be able to use this feature until we added it to UI somewhere. I'm wondering if we should add it to contextmenu right now, even if we later decide to move it elsewhere.

dai-shi · 2020-06-23T14:17:39Z

For me as a user, I'd be happy to have context menu anyway (not only about grid), even if we had another config UI.

dwelle · 2020-06-23T14:19:09Z

Yea, let's do it.

wip: grid for resizing a single element

974adb4

vercel bot deployed to Preview June 20, 2020 09:18 View deployment

wip: grid for dragging

fcda70c

vercel bot temporarily deployed to Preview June 20, 2020 23:29 Inactive

missing file to commit

855ba59

vercel bot deployed to Preview June 20, 2020 23:31 View deployment

dai-shi commented Jun 20, 2020

View reviewed changes

fix grid on mobile

65c9b8e

vercel bot deployed to Preview June 21, 2020 00:17 View deployment

create shapes on grids

6b8a66f

vercel bot deployed to Preview June 21, 2020 02:06 View deployment

dai-shi commented Jun 21, 2020

View reviewed changes

src/math.ts Outdated

return false;

};

export const pointOnGrids = (

Copy link

Member Author

dai-shi Jun 21, 2020

Choose a reason for hiding this comment

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

can suggest a better name?

shortcut to toggle grid

bc2f976

vercel bot deployed to Preview June 21, 2020 06:44 View deployment

dai-shi changed the title ~~[Not Yet For Merge] grid support~~ grid support (1st iteration) Jun 21, 2020

pshihn reviewed Jun 21, 2020

View reviewed changes

optimize drawing grids

f4da892

vercel bot deployed to Preview June 21, 2020 09:17 View deployment

dai-shi commented Jun 21, 2020

View reviewed changes

fix drag-alt duplication

9adbf03

vercel bot deployed to Preview June 22, 2020 22:44 View deployment

create two-point lines on grid

3a711cc

vercel bot deployed to Preview June 23, 2020 12:32 View deployment

dai-shi commented Jun 23, 2020

View reviewed changes

export grid size

666db87

vercel bot deployed to Preview June 23, 2020 13:11 View deployment

add to context menu

bd7f73f

vercel bot deployed to Preview June 23, 2020 14:48 View deployment

fix tests

072b224

vercel bot deployed to Preview June 23, 2020 15:21 View deployment

dwelle merged commit baa8fb6 into excalidraw:master Jun 23, 2020

dai-shi deleted the feat/grid-support branch June 23, 2020 15:26

lipis mentioned this pull request Jun 23, 2020

Umbrella: Grid mode issues #1810

Closed

5 tasks

This was referenced Jun 24, 2020

moving grouped elements jumps #1816

Closed

fix: start dragging grouped elements #1818

Merged

text resizing results in incorrect bounding box/position #1819

Closed

adjust font baseline on resize #1820

Merged

defint mentioned this pull request Aug 29, 2020

Configured grid size #2110

Open

maximecharriere mentioned this pull request Sep 4, 2024

Adding Grid Background Feature like Math Notebook Background #6773

Open

Uh oh!

Conversation

dai-shi commented Jun 20, 2020 • edited by dwelle Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

vercel bot commented Jun 20, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

dwelle commented Jun 20, 2020

Uh oh!

dai-shi commented Jun 20, 2020

Uh oh!

dai-shi commented Jun 20, 2020

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dai-shi commented Jun 21, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dwelle commented Jun 21, 2020

Uh oh!

dwelle commented Jun 21, 2020

Uh oh!

dai-shi commented Jun 21, 2020

Uh oh!

dwelle commented Jun 21, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

dwelle commented Jun 21, 2020

Uh oh!

dai-shi commented Jun 22, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dwelle commented Jun 23, 2020

Uh oh!

dwelle commented Jun 23, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

dai-shi commented Jun 23, 2020

Uh oh!

dwelle commented Jun 23, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

dai-shi commented Jun 23, 2020

Uh oh!

dwelle commented Jun 23, 2020

Uh oh!

dwelle commented Jun 23, 2020

Uh oh!

dai-shi commented Jun 23, 2020

Uh oh!

dwelle commented Jun 23, 2020

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

dai-shi commented Jun 20, 2020 •

edited by dwelle

Loading

vercel bot commented Jun 20, 2020 •

edited

Loading

dai-shi commented Jun 21, 2020 •

edited

Loading

dwelle commented Jun 21, 2020 •

edited

Loading

dai-shi commented Jun 22, 2020 •

edited

Loading

dwelle commented Jun 23, 2020 •

edited

Loading

dwelle commented Jun 23, 2020 •

edited

Loading