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 deleting newly placed components with Backspace key #771

Merged
merged 5 commits into from
Aug 16, 2022

Conversation

apedroferreira
Copy link
Member

@apedroferreira apedroferreira commented Aug 10, 2022

Fixes issue where it was not possible to delete a component from the editor with the "Backspace" key just after placing it, without unselecting and then selecting it again.
My solution to this issue was to refocus on the overlay element that catches keydown events after placing a new element, because dragging it from outside the editor was moving the focus outside of it.

Closes #752

@render
Copy link

render bot commented Aug 10, 2022

@oliviertassinari oliviertassinari requested a deployment to fix-deleting-newly-placed-component - toolpad-db PR #771 August 10, 2022 18:09 — with Render Abandoned
Signed-off-by: Pedro Ferreira <[email protected]>
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.

👍 makes sense, nice catch!

@oliviertassinari oliviertassinari added the bug 🐛 Something doesn't work label Aug 12, 2022
Comment on lines 1026 to 1029
const overlayElement = overlayRef.current;
if (overlayElement) {
overlayElement.focus();
}
Copy link
Member

@oliviertassinari oliviertassinari Aug 12, 2022

Choose a reason for hiding this comment

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

Do we need the defensive check here? I would assume React guarantees the ref is defined, hence:

Suggested change
const overlayElement = overlayRef.current;
if (overlayElement) {
overlayElement.focus();
}
overlayRef.current!.focus();

would be better, so we catch regressions (e.g. the ref is no longer bound to a DOM element) at runtime instead of going silent.

Copy link
Member

@Janpot Janpot Aug 13, 2022

Choose a reason for hiding this comment

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

In some other places we do:

Suggested change
const overlayElement = overlayRef.current;
if (overlayElement) {
overlayElement.focus();
}
invariant(overlayRef.current, 'Ref not bound');
overlayRef.current.focus();

This communicates the intent a bit clearer and avoids usage of !.

Finding a few more occurrences using regex search in vscode if \([^.]+\.current\b

Copy link
Member Author

Choose a reason for hiding this comment

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

good points, thanks! i used an invariant check here and also in a couple other places where it seemed right to use it

@render
Copy link

render bot commented Aug 16, 2022

@oliviertassinari oliviertassinari requested a deployment to fix-deleting-newly-placed-component - toolpad-db PR #771 August 16, 2022 12:50 — with Render Abandoned
@apedroferreira apedroferreira merged commit c54dac9 into master Aug 16, 2022
@apedroferreira apedroferreira deleted the fix-deleting-newly-placed-component branch August 16, 2022 13:34
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug 🐛 Something doesn't work
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Deleting component doesn't work when just added to the page
3 participants