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

Show border on hover in interactive nodes #1907

Merged
merged 5 commits into from
Apr 18, 2023

Conversation

apedroferreira
Copy link
Member

@apedroferreira apedroferreira commented Apr 17, 2023

Closes #1904

It was not possible to see the hover border for any ancestor/parent of the selected component anymore. That is the main issue fixed by this PR.
It was impossible to do this and keep pointer-events: none to be able to click through the overlay (the hover would also get disabled), so I've used a CSS clip path.

On top of this bug fix, we can also make it so that page rows and columns never show hover borders, or are selectable at all. Let me know if you think we should do that - it would not be difficult to add.

Before:

Screen.Recording.2023-04-17.at.22.07.34.mov

After:

Screen.Recording.2023-04-17.at.22.06.28.mov

@apedroferreira apedroferreira added bug 🐛 Something doesn't work regression A bug, but worse labels Apr 17, 2023
@apedroferreira apedroferreira self-assigned this Apr 17, 2023
@@ -174,14 +170,41 @@ export default function NodeHud({
}: NodeHudProps) {
const hintPosition = rect.y > HUD_HEIGHT ? HINT_POSITION_TOP : HINT_POSITION_BOTTOM;

const interactiveNodeClipPath = React.useMemo(
() =>
isInteractive && selectedNodeRect
Copy link
Member Author

Choose a reason for hiding this comment

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

The clip path is a polygon around the selection rectangle, as it's not possible do define inversed clip paths (would have been useful).

@apedroferreira apedroferreira marked this pull request as ready for review April 17, 2023 21:16
@@ -44,7 +42,7 @@ const NodeHudWrapper = styled('div', {
outline: `1px dotted ${isOutlineVisible ? theme.palette.primary[500] : 'transparent'}`,
zIndex: 80,
'&:hover': {
outline: `2px dashed ${isHoverable ? 'transparent' : theme.palette.primary[500]}`,
outline: `2px dashed ${isHoverable ? theme.palette.primary[500] : 'transparent'}`,
Copy link
Member Author

Choose a reason for hiding this comment

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

This was wrong too, but it's unrelated.

@apedroferreira apedroferreira requested a review from a team April 17, 2023 21:24
Copy link
Member

@Janpot Janpot left a comment

Choose a reason for hiding this comment

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

Not sure how hard it would be to add a test for this, but it seems fine.

This overlay is becoming quite complex. I sometimes wonder whether there's a simpler way to implement it. Instead of creating nested elements that mirror the underlying page hierarchy, read the mouse position and determine which is the closest element underneath. Then only create geometry for that rectangle.

@apedroferreira
Copy link
Member Author

On top of this bug fix, we can also make it so that page rows and columns never sho

If this fails elements such as text inputs would not be interactive in the editor, so I think it's covered by the tests.

About the complexity, for this particular issue I tried a lot of solutions and this was the first one that worked - but on a larger scale when working on this logic I always kind of followed what was already there, so we might want to try to refactor it from the ground up later and make it simpler - it should be possible.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug 🐛 Something doesn't work regression A bug, but worse
Projects
None yet
Development

Successfully merging this pull request may close these issues.

PageRow is not selectable
2 participants