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: Make whole element clickable in view mode when it has hyperlink #4735

Merged
merged 5 commits into from
Feb 7, 2022

Conversation

ad1992
Copy link
Member

@ad1992 ad1992 commented Feb 7, 2022

ScreenFlow.mp4

@vercel
Copy link

vercel bot commented Feb 7, 2022

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/2cEFL6XzHYwePk8sNGhaFFLsteZR
✅ Preview: https://excalidraw-git-aakansha-link-view-excalidraw.vercel.app

@ad1992 ad1992 requested a review from dwelle February 7, 2022 09:23
@dwelle
Copy link
Member

dwelle commented Feb 7, 2022

Cool. One thing: we should disable click-through if you drag between pointerdown and pointerup, if the drag distance is above some threshold (e.g. the DRAGGING_THRESHOLD we're using). Otherwise it will open links if you start dragging and release over a link. Can happen often when the links are on elements, not text.

excal-view-mode-link-drag-bug

(This affects edit mode as well, but to a much smaller degree since the clickable hitboxes are only the small icons.)

@ad1992
Copy link
Member Author

ad1992 commented Feb 7, 2022

DRAGGING_THRESHOLD

I am instead checking the pointer down and pointer up coords are exactly same as for click event it should be same then only redirect

@dwelle
Copy link
Member

dwelle commented Feb 7, 2022

I am instead checking the pointer down and pointer up coords are exactly same as for click event it should be same then only redirect

exact match is too strict. I tend to make micro movements a lot when clicking using a mouse.

@ad1992
Copy link
Member Author

ad1992 commented Feb 7, 2022

I am instead checking the pointer down and pointer up coords are exactly same as for click event it should be same then only redirect

exact match is too strict. I tend to make micro movements a lot when clicking using a mouse.

But when its a click the pointer events will be same else it means that there was some movement?

@dwelle
Copy link
Member

dwelle commented Feb 7, 2022

But when its a click the pointer events will be same else it means that there was some movement?

There is a tiny movement, yes. Hence the DRAGGING_THRESHOLD (10px), which we consider as unintentional.

@ad1992 ad1992 merged commit 66c92fc into master Feb 7, 2022
@ad1992 ad1992 deleted the aakansha-link-view branch February 7, 2022 14:24
@yeger00
Copy link

yeger00 commented Apr 16, 2023

Hello,
Small question about this PR - what is the reason behind commit b21abd5 - disable the feature on mobile? It might make a difference for an app I'm working on, and I disabled it on a local branch and it seems to be working as expected.

Thanks,
Avi

@ad1992
Copy link
Member Author

ad1992 commented Apr 19, 2023

Hello, Small question about this PR - what is the reason behind commit b21abd5 - disable the feature on mobile? It might make a difference for an app I'm working on, and I disabled it on a local branch and it seems to be working as expected.

Thanks, Avi

@yeger00 the only reason for not making the element clickable in mobile was since there isn't any tooltip when you hover over the item so it might not be very intutive that it would open a link as soon as you click on it.
Would like to know your reasoning behind enabling it on mobile and based on that we can surely revisit this :)

@yeger00
Copy link

yeger00 commented Apr 22, 2023

@ad1992 Thank you for your comment.
This is still a WIP, but I'm using Excalidraw as the platform to easily create "roadmap" for training. As an example, I'm working on some general "web development" roadmap here (and the code is here), but I plan to use it a training platform for newcomers in my work as well.
In this example, I patched both the "click" in mobile, and removed the blue icon for links (as suggested here).

Let me know what you think.
Thanks,
Avi

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

3 participants