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

More mobile tweaks #790

Merged
merged 18 commits into from Feb 21, 2020
Merged

More mobile tweaks #790

merged 18 commits into from Feb 21, 2020

Conversation

j-f1
Copy link
Member

@j-f1 j-f1 commented Feb 21, 2020

Ref #138

  • language selector
  • remove lock button on mobile

@vercel
Copy link

vercel bot commented Feb 21, 2020

This pull request is being automatically deployed with ZEIT Now (learn more).
To see the status of your deployment, click below or on the icon next to each commit.

🔍 Inspect: https://zeit.co/vjeux/excalidraw/ih9g0u3t1
✅ Preview: https://excalidraw-git-more-tweaks.vjeux.now.sh

@klikstermkd klikstermkd mentioned this pull request Feb 21, 2020
13 tasks
@dwelle

This comment has been minimized.

@j-f1

This comment has been minimized.

@dwelle

This comment has been minimized.

@dwelle

This comment has been minimized.

@j-f1

This comment has been minimized.

@vjeux
Copy link
Contributor

vjeux commented Feb 21, 2020

The done button for drawing arrows seems to be off by one. It only shows after you have added three points instead of two. When you confirm, it drops the last segment of the arrow

@dwelle

This comment has been minimized.

@vjeux

This comment has been minimized.

@vjeux

This comment has been minimized.

@j-f1

This comment has been minimized.

@vjeux
Copy link
Contributor

vjeux commented Feb 21, 2020

There’s a small jump while dragging elements

  • dezoom to the maximum (to make it more obvious)
  • create a circle
  • touch down on the edge with one finger
  • move by one or two pixels
  • the circle jumps to more than a few pixels
  • move by one or two more pixels
  • the circle properly tracks your finger

Usually when I see this kind of bugs, it’s an issue where we don’t keep track of the offset where the element was started being dragged and instead we use the center.

@dwelle

This comment has been minimized.

@vjeux

This comment has been minimized.

@dwelle

This comment has been minimized.

@vjeux

This comment has been minimized.

@vjeux
Copy link
Contributor

vjeux commented Feb 21, 2020

(Just to be clear, I’m just posting here all the things I’m seeing around mobile support, they are not required to be addressed in this PR)

@j-f1
Copy link
Member Author

j-f1 commented Feb 21, 2020

There is no longer an edit icon to toggle it off

Fixed.

The done button for drawing arrows seems to be off by one. [...] When you confirm, it drops the last segment of the arrow

If you use the mouse to click two segments, then move to a third point without clicking, then hit enter, you’ll see the same thing. Not sure how to resolve it.

@dwelle
Copy link
Member

dwelle commented Feb 21, 2020

The done button for drawing arrows seems to be off by one. [...] When you confirm, it drops the last segment of the arrow

If you use the mouse to click two segments, then move to a third point without clicking, then hit enter, you’ll see the same thing. Not sure how to resolve it.

That's a different thing with the mouse. The last point isn't committed yet, so it's correct that upon confirm it's discarded.

@j-f1
Copy link
Member Author

j-f1 commented Feb 21, 2020

actionFinalize removes the last point from the multi-point object it’s passed. This is because when you click with a mouse, another segment is automatically added that follows the cursor. How can I reconcile these two behaviors?

@dwelle
Copy link
Member

dwelle commented Feb 21, 2020

I guess conditionally remove the last point only if not on mobile.

@j-f1
Copy link
Member Author

j-f1 commented Feb 21, 2020

I’m also thinking of cases like Windows laptops where you might lay points down with a mouse or touch.

@j-f1
Copy link
Member Author

j-f1 commented Feb 21, 2020

I think the best option would be to record the pointer the most recent point was laid down with and only remove it if the pointer is a mouse. How would I store that so the finalize action can access it but it isn’t permanently stored in the document?

@j-f1
Copy link
Member Author

j-f1 commented Feb 21, 2020

There’s a small jump while dragging elements

I can’t reproduce this on an iPhone 8 or simulated iPhone 11.

@dwelle
Copy link
Member

dwelle commented Feb 21, 2020

I think the best option would be to record the pointer the most recent point was laid down with and only remove it if the pointer is a mouse. How would I store that so the finalize action can access it but it isn’t permanently stored in the document?

I haven't checked the code. Yes, the best way is to identify whether you're in the created-temp-arrow-point mode and act on that.

@j-f1
Copy link
Member Author

j-f1 commented Feb 21, 2020

This might also be related to the fact that tapping once to start a line actually creates two points.

@vjeux
Copy link
Contributor

vjeux commented Feb 21, 2020

Is this in a state to be merged in? If yes, we should probably do it and continue support in a follow up.

@lipis
Copy link
Member

lipis commented Feb 21, 2020

ZcP51VCV

Strange focus border when tapping on hamburger menu on Pixel 4..

@j-f1
Copy link
Member Author

j-f1 commented Feb 21, 2020

@lipis Weird — I can’t repro on iOS. I’ll try to use an Android emulator in the future, but I suspect it’ll be fixed by switching those buttons to be radio buttons, which I plan to do as part of a future refactor.

@j-f1
Copy link
Member Author

j-f1 commented Feb 21, 2020

Good to go @vjeux — I moved the incomplete tasks back to the issue.

@lipis lipis marked this pull request as ready for review February 21, 2020 19:19
@vjeux vjeux merged commit 0fd3fb4 into master Feb 21, 2020
@vjeux vjeux deleted the more-tweaks branch February 21, 2020 19:34
@j-f1 j-f1 marked this pull request as ready for review February 21, 2020 19:41
@j-f1 j-f1 linked an issue Feb 21, 2020 that may be closed by this pull request
13 tasks
@vjeux
Copy link
Contributor

vjeux commented Feb 24, 2020

I disagree that the mobile UI should have less features because it’s on a phone. I use the lock icon a lot when drawing shapes with a bunch of lines. I wish we didn’t totally remove it as it’s useful.

I’m happy for it being behind a menu.

vjeux added a commit to vjeux/excalidraw that referenced this pull request Dec 6, 2020
For rendering we always use mouse in order to check which handles to display but when doing the hit test, we used pointer which has a different size. So we couldn't use the middle handles for small shapes. This is now fixed.

cc @j-f1 as you added it in excalidraw#790
vjeux added a commit that referenced this pull request Dec 6, 2020
For rendering we always use mouse in order to check which handles to display but when doing the hit test, we used pointer which has a different size. So we couldn't use the middle handles for small shapes. This is now fixed.

cc @j-f1 as you added it in #790
amilajack pushed a commit to palettedev/excalidraw that referenced this pull request Mar 6, 2023
* Edit Farsi translations (excalidraw#788)

* Add a Ukrainian translation (excalidraw#786)

* Add a Ukrainian translation

* Clarify some strings in the Ukrainian translation

* feat: change dock position (excalidraw#774)

* feat: change dock position

* fix grid row and column

* add top position

* fix responsive for the top position

* change content side

* fix overflowing menu

* [improvement] theme on body (excalidraw#790)

* Update Tldraw.tsx

* Add theme on body, adjust dark page options dialog

* fix test

* Preparing for global integration (excalidraw#775)

* Update translations.ts

* Create en.json

* Make main translation default

* Remove unused locale property of translation

Co-authored-by: Steve Ruiz <steveruizok@gmail.com>

* Fix language menu

* Update ar.json (excalidraw#793)

* feature/add Hebrew translations (excalidraw#792)

* hebrew translations

* pr fixes

Co-authored-by: Steve Ruiz <steveruizok@gmail.com>

* fix toolspanel item position (excalidraw#791)

* fix toolspanel item position

* add translation

Co-authored-by: Steve Ruiz <steveruizok@gmail.com>

* Add remote caching

* Adds link to translation guide (excalidraw#794)

Co-authored-by: Baahar Ebrahimi <108254874+Baahaarmast@users.noreply.github.com>
Co-authored-by: walking-octopus <46994949+walking-octopus@users.noreply.github.com>
Co-authored-by: Judicael <46365844+judicaelandria@users.noreply.github.com>
Co-authored-by: Ali Alhaidary <75235623+ali-alhaidary@users.noreply.github.com>
Co-authored-by: gadi246 <gadi246@gmail.com>
amilajack pushed a commit to palettedev/excalidraw that referenced this pull request Mar 6, 2023
* Edit Farsi translations (excalidraw#788)

* Add a Ukrainian translation (excalidraw#786)

* Add a Ukrainian translation

* Clarify some strings in the Ukrainian translation

* feat: change dock position (excalidraw#774)

* feat: change dock position

* fix grid row and column

* add top position

* fix responsive for the top position

* change content side

* fix overflowing menu

* [improvement] theme on body (excalidraw#790)

* Update Tldraw.tsx

* Add theme on body, adjust dark page options dialog

* fix test

* Preparing for global integration (excalidraw#775)

* Update translations.ts

* Create en.json

* Make main translation default

* Remove unused locale property of translation

Co-authored-by: Steve Ruiz <steveruizok@gmail.com>

* Fix language menu

* Update ar.json (excalidraw#793)

* feature/add Hebrew translations (excalidraw#792)

* hebrew translations

* pr fixes

Co-authored-by: Steve Ruiz <steveruizok@gmail.com>

* fix toolspanel item position (excalidraw#791)

* fix toolspanel item position

* add translation

Co-authored-by: Steve Ruiz <steveruizok@gmail.com>

* Add remote caching

* Adds link to translation guide (excalidraw#794)

* Update ar.json (excalidraw#795)

* [feature] readonly link (excalidraw#796)

* Copy readonly link

* Update [id].tsx

* Add readonly label

* update psuedohash

* Update utils.ts

Co-authored-by: Baahar Ebrahimi <108254874+Baahaarmast@users.noreply.github.com>
Co-authored-by: walking-octopus <46994949+walking-octopus@users.noreply.github.com>
Co-authored-by: Judicael <46365844+judicaelandria@users.noreply.github.com>
Co-authored-by: Ali Alhaidary <75235623+ali-alhaidary@users.noreply.github.com>
Co-authored-by: gadi246 <gadi246@gmail.com>
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.

Mobile support
4 participants