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

Make it possible to opt out of focus handling #90

Open
wants to merge 7 commits into
base: main
Choose a base branch
from

Conversation

Haraldson
Copy link

@Haraldson Haraldson commented Oct 4, 2021

Possible bonus: Regard contenteditables as interactive elements, but I understand if you don’t want this as part of the same PR, or have considered this before and concluded that it’s best to leave them out.

I’m not entirely sure about the flow of arguments here and whether it could be simplified or flow in a more natural way, but it seems to be working as intended.

Fixes #89

@Haraldson Haraldson marked this pull request as ready for review October 4, 2021 21:35
@Haraldson
Copy link
Author

Haraldson commented Oct 4, 2021

Did a lil’ self-review and noticed my brain did a derp. This code will work as intended in my use case, or for people who explicitly defined the prop, but not for anyone else.

The JavaScript flavor here seems fairly modern, but I don’t see any default values in function signatures, null-coalescing operators or similar. What’s your preferred way of defining default values, @rafgraph?

`handleFocus` (needed to be set to `true` to retain current focus behavior, a breaking change) -> `preventFocusHandling` (needs to be explicitly set to `true` to opt out of current focus behavior, non-breaking).
@rafgraph
Copy link
Owner

rafgraph commented Oct 7, 2021

Thanks for the PR. Generally looks good, thanks for contenteditable addition.

One important change, the preventFocusHandling prop should be tracked as a singleton (like hashFragment on line 5) and not passed to the functions.

src/HashLink.jsx Outdated Show resolved Hide resolved
src/HashLink.jsx Outdated Show resolved Hide resolved
@Haraldson
Copy link
Author

I hope and think that should do it, @rafgraph!

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.

Should be an option to disable focus management
2 participants