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

Add explicit focusOptions support for ReactFocusLock #63

Merged
merged 2 commits into from
Nov 13, 2022
Merged

Add explicit focusOptions support for ReactFocusLock #63

merged 2 commits into from
Nov 13, 2022

Conversation

cee-chen
Copy link
Contributor

closes #62

As of 2.7.0, react-focus-lock has supported a focusOptions prop, primarily for allowing consumers to pass preventScroll to the initial focus event. react-focus-on unfortunately has not exposed passing that prop directly to react-focus-lock, which potentially leads to scroll jumping issues that cannot be overridden (see theKashey/react-focus-lock#162 for more info).

I looked but couldn't see any meaningful unit tests to write/update to include this new prop - please feel free to ping me if there are!

@theKashey
Copy link
Owner

Hey. Thank you for your contribution and the wonderful debugging journey.

Wondering what would be the best approach here:

  • exposing low level focusOption property - might be too implementation detail-ish
  • what about a higher level property focusScroll={false} or scrollOnFocus="prevented"
    • naming is always hard 😞

One of the reasons to prefer more abstract solutions - easier to correct "default behaviour" on a major version bump.

Thoughts?

@theKashey theKashey self-requested a review October 30, 2022 09:22
@cee-chen
Copy link
Contributor Author

cee-chen commented Oct 30, 2022

I think I'd lean towards a boolean to match native behavior over a 'prevented' | undefined API personally. How do you feel about scrollOnFocus={false} (defaulting to true)?

Totally understand the difficulties of balancing flexibility vs abstraction. I can update my PR on Monday with whatever API you prefer.

@theKashey
Copy link
Owner

Proposal sounds reasonable. What about actually matching underlying primitive as preventScrollOnFocus={true}, as "false props" are not your friends?

@cee-chen
Copy link
Contributor Author

I'm good either way! I pushed up the preventScrollOnFocus API change - let me know if that works for you or if you have any change requests!

Copy link
Owner

@theKashey theKashey left a comment

Choose a reason for hiding this comment

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

Let me handle the rest

@cee-chen
Copy link
Contributor Author

Thanks a million! I don't have merge/write access to merge the PR as a heads up.

@theKashey theKashey merged commit 7a2900f into theKashey:master Nov 13, 2022
@theKashey
Copy link
Owner

Released in 3.7.0

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.

Support passing focusOptions to react-focus-lock
2 participants