-
-
Notifications
You must be signed in to change notification settings - Fork 367
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: Fix PlaygroundEditor
preventing scroll when not focused
#974
Conversation
This pull request is being automatically deployed with Vercel (learn more). ariakit – ./🔍 Inspect: https://vercel.com/ariakit/ariakit/4kBtsDCaaXVpgmhQo1H5u379cq72 reakit – ./🔍 Inspect: https://vercel.com/ariakit/reakit/3bwPdcV8SxyedpHpxb23a7JQNQEX |
This pull request is automatically built and testable in CodeSandbox. To see build info of the built libraries, click here or the icon next to each commit SHA. |
Codecov Report
@@ Coverage Diff @@
## v2 #974 +/- ##
=======================================
Coverage 62.63% 62.63%
=======================================
Files 159 159
Lines 4129 4129
Branches 1159 1159
=======================================
Hits 2586 2586
Misses 1542 1542
Partials 1 1 Continue to review full report at Codecov.
|
@@ -212,7 +212,7 @@ export type PlaygroundCodeOptions<T extends As = "div"> = Options<T> & { | |||
maxHeight?: number; | |||
disclosureProps?: ButtonProps<ElementType>; | |||
expanded?: boolean; | |||
setExpanded?: SetState<boolean>; | |||
setExpanded?: (expanded: boolean) => void; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Changing this type so it doesn't need to be a state updater function similar to the one returned by React's useState
. That is, people can pass any stable or non-stable function that accepts a boolean value.
const [expanded, setExpanded] = useControlledState( | ||
defaultExpanded ?? !maxHeight, | ||
expandedProp, | ||
setExpandedProp | ||
); | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Defining the state here so we can access the expanded state in the PlaygroundEditor
component. PlaygroundEditor
uses PlaygroundCode
underneath and extends its prop types. Since PlaygroundEditor
can also receive the expanded props, we're doing the same thing here as in the PlaygroundEditor
component.
@@ -241,7 +251,7 @@ export const usePlaygroundEditor = createHook<PlaygroundEditorOptions>( | |||
if (event.defaultPrevented) return; | |||
setFocusVisible(true); | |||
}, | |||
[onFocusVisibleProp, editable] | |||
[onFocusVisibleProp] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just a cleanup. Not sure why this was here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍
Right now, we can't scroll the playground editor when it's expanded, but not focused (which in the code is translated into the
editable
local state).This has been an annoying experience. So this PR fixes the behavior so the editor doesn't need to be focused anymore to be scrollable.
How to test
And then repeat the same with this PR preview site (https://ariakit-git-fix-playground-editor-scrolling-ariakit.vercel.app/examples/combobox)