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: improve elbow arrow keyboard move #8392

Merged
merged 7 commits into from
Aug 26, 2024

Conversation

zsviczian
Copy link
Collaborator

Fixes: Keyboard arrow keys stop to move elements when the group includes an elbow arrow #8387

Having looked at the code I concluded that this is not a bug, but a too restrictive feature. However, we can make the restriction more targeted/relaxed even without touching the arrow move logic. The keyboard move should only be restricted if the start or end element is not part of the selection.

Copy link

vercel bot commented Aug 18, 2024

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Updated (UTC)
excalidraw ✅ Ready (Inspect) Visit Preview Aug 18, 2024 3:02pm
excalidraw-package-example ✅ Ready (Inspect) Visit Preview Aug 18, 2024 3:02pm
excalidraw-package-example-with-nextjs ✅ Ready (Inspect) Visit Preview Aug 18, 2024 3:02pm
1 Skipped Deployment
Name Status Preview Updated (UTC)
docs ⬜️ Ignored (Inspect) Visit Preview Aug 18, 2024 3:02pm

Copy link

github-actions bot commented Aug 18, 2024

Coverage Report

Status Category Percentage Covered / Total
🔴 Lines 66.6% (🎯 70%) 58291 / 87524
🔴 Statements 66.6% (🎯 70%) 58291 / 87524
🟢 Functions 68.29% (🎯 68%) 1734 / 2539
🟢 Branches 80.98% (🎯 70%) 7292 / 9004
File Coverage
File Stmts % Branch % Funcs % Lines Uncovered Lines
Changed Files
packages/excalidraw/components/App.tsx 70.33% 77.42% 69.74% 70.33% 485-486, 596-605, 704-705, 723-724, 745-805, 808-814, 827-830, 833-909, 912-931, 934-939, 947-957, 959-960, 965-966, 970-972, 986, 993-1255, 1315-1316, 1348-1350, 1357-1402, 1428, 1438, 1445-1448, 1457-1461, 1493-1494, 1578-1588, 1593-1608, 1612-1659, 1734-1739, 1768-1773, 1776-1806, 1814-1839, 1842-1851, 1854-1966, 1969-1977, 1986-1999, 2002-2028, 2031-2102, 2105-2146, 2149-2154, 2191-2192, 2214-2215, 2246-2247, 2251-2252, 2272-2280, 2285-2298, 2304-2305, 2309, 2314-2322, 2324-2332, 2344, 2386-2387, 2409-2410, 2417, 2490-2492, 2495-2500, 2505-2506, 2546-2554, 2559-2568, 2606-2607, 2694-2695, 2699, 2702-2703, 2711-2714, 2723-2736, 2742-2745, 2748, 2750-2751, 2758-2759, 2765-2766, 2769-2770, 2778-2779, 2782-2783, 2794-2802, 2807-2808, 2859-2860, 2874-2880, 2886-2894, 2898-2906, 2910-2911, 2914-2947, 2950-2962, 2973-2974, 2985-2986, 3003-3007, 3011-3014, 3021-3023, 3041-3048, 3051, 3053-3058, 3062-3064, 3110-3111, 3118-3120, 3122-3145, 3171, 3177, 3233-3234, 3251-3253, 3275-3276, 3282-3285, 3291-3372, 3426, 3430, 3465-3466, 3525, 3535-3553, 3556-3562, 3565-3566, 3572-3588, 3693-3694, 3717-3731, 3736-3755, 3807-3808, 3888-3893, 3924, 4050-4051, 4053-4060, 4094-4098, 4100-4101, 4103-4106, 4131-4132, 4154-4164, 4174-4176, 4178, 4259-4262, 4284-4292, 4296-4298, 4308-4309, 4311-4331, 4338-4361, 4364-4370, 4387-4390, 4396-4399, 4409-4416, 4511-4515, 4519, 4524-4525, 4530-4534, 4567-4568, 4571-4572, 4584-4588, 4593-4594, 4600-4610, 4615-4642, 4647-4658, 4755-4756, 4840, 4865, 4891-4893, 4960-4961, 4979-4980, 5155-5156, 5159-5160, 5168-5169, 5219-5223, 5277-5284, 5290-5356, 5400, 5457, 5484, 5491-5492, 5505-5508, 5539, 5587-5590, 5593-5599, 5601-5604, 5621-5630, 5633-5634, 5754-5755, 5758, 5760-5765, 5771-5773, 5775, 5784, 5806-5811, 5813-5816, 5820-5821, 5831-5931, 5935-5936, 5951-5952, 5988-5994, 6021-6022, 6053, 6055-6082, 6089-6090, 6112-6113, 6132, 6134-6177, 6182-6183, 6185-6186, 6202-6203, 6209-6210, 6214-6217, 6220-6221, 6235-6262, 6270-6271, 6275-6278, 6280-6284, 6302-6306, 6353-6358, 6360-6362, 6378, 6380-6393, 6418-6421, 6465, 6482-6483, 6485-6508, 6523-6524, 6638-6666, 6772-6773, 6793-6794, 6888, 6894-6895, 6918-6937, 6952, 7081, 7103-7141, 7145-7195, 7210, 7219, 7253-7259, 7285-7287, 7454-7457, 7479-7509, 7522, 7524-7532, 7546, 7548-7556, 7563-7566, 7574-7579, 7606-7607, 7612-7614, 7617-7618, 7643-7644, 7675-7676, 7759-7760, 7807-7820, 7897-7900, 7926-7927, 7983, 7998-8004, 8011-8025, 8049-8055, 8087, 8129, 8135, 8150-8157, 8160-8167, 8223, 8301-8302, 8323-8325, 8328, 8342-8366, 8470-8483, 8504-8528, 8537-8576, 8589-8590, 8599-8606, 8624-8631, 8685-8711, 8713-8714, 8788-8789, 8796, 8798-8834, 8863, 8922, 8956-8958, 8983-8988, 8990-8991, 8996-8998, 9001-9021, 9035-9036, 9042-9051, 9056, 9060-9064, 9073-9077, 9080-9085, 9089-9096, 9126, 9128-9132, 9134-9144, 9160-9162, 9173-9181, 9185-9229, 9232-9303, 9311, 9329-9336, 9338-9354, 9369-9391, 9410-9412, 9458-9459, 9575-9579, 9581-9597, 9599-9603, 9615-9616, 9626-9642, 9661-9680, 9682-9683, 9710-9711, 9715-9716, 9726, 9728-9731, 9733-9734, 9774, 9813-9814, 9824, 9866, 9879-9887, 9908-9909, 9941-9944, 10037-10044, 10070-10071, 10114-10168, 10216-10217, 10226-10229, 10234-10235, 10256-10258, 10260-10264, 10302
Generated in workflow #3105

Copy link
Collaborator

@mtolmacs mtolmacs left a comment

Choose a reason for hiding this comment

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

This solves the specific issue this PR references, but don't we want to allow moving elbow arrows bound on only one end if the connected end boundable is also in the selection @dwelle ?

packages/excalidraw/components/App.tsx Outdated Show resolved Hide resolved
packages/excalidraw/components/App.tsx Outdated Show resolved Hide resolved
packages/excalidraw/components/App.tsx Show resolved Hide resolved
packages/excalidraw/components/App.tsx Show resolved Hide resolved
@zsviczian
Copy link
Collaborator Author

This solves the specific issue this PR references, but don't we want to allow moving elbow arrows bound on only one end if the connected end boundable is also in the selection @dwelle ?

I agree - that would be ideal, however, this PR is already a big improvement.

@zsviczian
Copy link
Collaborator Author

This solves the specific issue this PR references, but don't we want to allow moving elbow arrows bound on only one end if the connected end boundable is also in the selection @dwelle ?

I agree - that would be ideal, however, this PR is already a big improvement.

I've modified the solution to include @mtolmacs's case as well. I think this is much better from a UX perspective.

@mtolmacs
Copy link
Collaborator

I can live with this, but I defer to @dwelle to give final approval. Thanks for the fix and flexibility!

@dwelle
Copy link
Member

dwelle commented Aug 19, 2024

I can live with this, but I defer to @dwelle to give final approval. Thanks for the fix and flexibility!

Haven't checked code, but from checking the preview it looks now arrow behavior is aligned to dragging behavior, which is what we want? Or is there something I'm missing?

@mtolmacs
Copy link
Collaborator

I can live with this, but I defer to @dwelle to give final approval. Thanks for the fix and flexibility!

Haven't checked code, but from checking the preview it looks now arrow behavior is aligned to dragging behavior, which is what we want? Or is there something I'm missing?

Yes, it makes sense to have the same behavior IMO.

@zsviczian
Copy link
Collaborator Author

Just to confirm, the current code in the PR fully aligns keyboard move with the dragging behavior for elbow arrows.

Copy link
Member

@dwelle dwelle left a comment

Choose a reason for hiding this comment

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

thanks!

@dwelle dwelle merged commit afb68a6 into master Aug 26, 2024
10 checks passed
@dwelle dwelle deleted the zsviczian-improve-elbowarrowkeyboardmove branch August 26, 2024 13:58
shenyih0ng pushed a commit to shenyih0ng/excalidraw that referenced this pull request Aug 27, 2024
clarencechaan pushed a commit to clarencechaan/excalidraw that referenced this pull request Oct 3, 2024
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.

3 participants