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

fix(visual-editing): unset overlay applied cursor on mouseleave #2181

Merged
merged 4 commits into from
Nov 20, 2024

Conversation

rdunk
Copy link
Member

@rdunk rdunk commented Nov 18, 2024

Fix for #2179. This unsets the cursor style when mouseleave-ing an overlay element. Previously on mouseleave the element was specified as the hoveredElement, which I think would be the element that the cursor was entering after leaving the one we want to reset?

Two questions:

  1. Should we only set cursor to auto on child elements of drag groups?
  2. As this manipulates the DOM outside of our visual editing element, should we store and revert to any user set cursor style value?

@georgedoescode I've assigned this to you as you'll know the original intention!

@rdunk rdunk requested a review from a team as a code owner November 18, 2024 20:40
Copy link

vercel bot commented Nov 18, 2024

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

Name Status Preview Comments Updated (UTC)
live-visual-editing-next ✅ Ready (Inspect) Visit Preview 💬 Add feedback Nov 20, 2024 3:48pm
visual-editing-astro ✅ Ready (Inspect) Visit Preview 💬 Add feedback Nov 20, 2024 3:48pm
visual-editing-next ✅ Ready (Inspect) Visit Preview 💬 Add feedback Nov 20, 2024 3:48pm
visual-editing-next-with-i18n ✅ Ready (Inspect) Visit Preview 💬 Add feedback Nov 20, 2024 3:48pm
visual-editing-nuxt ✅ Ready (Inspect) Visit Preview 💬 Add feedback Nov 20, 2024 3:48pm
visual-editing-page-builder-demo ✅ Ready (Inspect) Visit Preview 💬 Add feedback Nov 20, 2024 3:48pm
visual-editing-remix ✅ Ready (Inspect) Visit Preview 💬 Add feedback Nov 20, 2024 3:48pm
visual-editing-storybook ✅ Ready (Inspect) Visit Preview 💬 Add feedback Nov 20, 2024 3:48pm
visual-editing-svelte ✅ Ready (Inspect) Visit Preview 💬 Add feedback Nov 20, 2024 3:48pm
1 Skipped Deployment
Name Status Preview Comments Updated (UTC)
visual-editing-studio ⬜️ Skipped (Inspect) Nov 20, 2024 3:48pm

@rdunk rdunk requested review from georgedoescode and removed request for a team November 18, 2024 20:40
@rdunk
Copy link
Member Author

rdunk commented Nov 20, 2024

Pushed a new commit which implements both the suggestions in the original description.

// cursor to 'move'. Otherwise the element is a child ancestor, so set
// it to 'auto' to override the parent's move cursor
const isHoveredElement = element === el
cursor = isHoveredElement ? 'move' : 'auto'
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm wondering — do we need to modify the children's cursor values? I feel like if we don't need to show the drag handle, we can just do nothing?

Copy link
Member Author

Choose a reason for hiding this comment

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

Huh, this is a good point actually... This workaround of checking the hover stack is necessary because our deferred leave pattern means the leave handler isn't triggered until the element is removed from the hover stack, so the parent element maintains its cursor style and the child elements need to use auto to override the cascade.

It might be possible to just not set the cursor style in the deferred leave handler, but in the mouseleave event itself. That would simplify things.

if (cursor) {
element.style.cursor = cursor
} else {
element.style.removeProperty('cursor')
Copy link
Contributor

Choose a reason for hiding this comment

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

Related to the above: if we don't modify children's cursor values, we may not need this removeProperty?

Copy link
Member Author

Choose a reason for hiding this comment

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

We'd still need to remove the property for the elements set to move though right?

Copy link
Contributor

Choose a reason for hiding this comment

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

Ah yes, I was thinking that the function you added that stores the original value would always reset it anyway, but perhaps not if there was not a cursor set 👍

Copy link
Contributor

@georgedoescode georgedoescode left a comment

Choose a reason for hiding this comment

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

This is working really well. Nice one!

@rdunk rdunk merged commit 2d39ab7 into main Nov 20, 2024
20 checks passed
@rdunk rdunk deleted the fix/set-cursor branch November 20, 2024 16:12
@ecospark ecospark bot mentioned this pull request Nov 20, 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.

2 participants