theKashey / react-focus-on

🎯 Solution for WAI ARIA compatible modal dialogs or full-screen tasks, you were looking for
MIT License
333 stars 14 forks source link

Add explicit `focusOptions` support for `ReactFocusLock` #63

Closed cee-chen closed 1 year ago

cee-chen commented 1 year ago

closes https://github.com/theKashey/react-focus-on/issues/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 https://github.com/theKashey/react-focus-lock/issues/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 commented 1 year ago

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

Wondering what would be the best approach here:

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

Thoughts?

cee-chen commented 1 year ago

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 commented 1 year ago

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

cee-chen commented 1 year ago

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!

cee-chen commented 1 year ago

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

theKashey commented 1 year ago

Released in 3.7.0