feat: Add SlideIn component#3893
Merged
Merged
Conversation
Signed-off-by: Adriano Lazzaretti <lazzaretti136@gmail.com>
Contributor
There was a problem hiding this comment.
Hey there - I've reviewed your changes - here's some feedback:
- The
createPortal(slideInContent, document.body)call assumesdocumentis always available, which will break in SSR/Node environments; consider guarding with atypeof document !== 'undefined'check or deferring portal creation until after mount. - The open/close animation duration is duplicated in the CSS classes (
duration-300) and thesetTimeoutused to unmount (300ms); extracting this into a shared constant will reduce the risk of desynchronization if the timing is updated later.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- The `createPortal(slideInContent, document.body)` call assumes `document` is always available, which will break in SSR/Node environments; consider guarding with a `typeof document !== 'undefined'` check or deferring portal creation until after mount.
- The open/close animation duration is duplicated in the CSS classes (`duration-300`) and the `setTimeout` used to unmount (300ms); extracting this into a shared constant will reduce the risk of desynchronization if the timing is updated later.
## Individual Comments
### Comment 1
<location> `turboui/src/SlideIn/index.tsx:46-55` </location>
<code_context>
+ setIsAnimating(true);
+ }, 10);
+
+ // Disable body scroll when slide-in is open
+ document.body.style.overflow = "hidden";
+
+ // Handle ESC key to close the slide-in
+ const handleEsc = (event: KeyboardEvent) => {
+ if (event.key === "Escape" && !event.defaultPrevented) {
+ onClose();
+ }
+ };
+
+ document.addEventListener("keydown", handleEsc);
+
+ return () => {
+ clearTimeout(timer);
+ document.body.style.overflow = "";
+ document.removeEventListener("keydown", handleEsc);
+ };
</code_context>
<issue_to_address>
**suggestion:** Consider delaying body scroll restoration until after the closing animation finishes.
Right now `document.body.style.overflow = ""` runs as soon as `isOpen` becomes false, while the panel remains visible for ~300ms. This lets the background scroll while the panel is still animating out. Consider deferring the overflow reset to the closing timeout (or tracking a separate "visible" state) so body scroll is only restored after the animation completes.
Suggested implementation:
```typescript
document.addEventListener("keydown", handleEsc);
return () => {
clearTimeout(timer);
document.removeEventListener("keydown", handleEsc);
};
} else {
// Trigger close animation
```
To fully implement the suggestion you should:
1. In the `else` branch (where you trigger the close animation), add the `document.body.style.overflow = ""` call in the timeout that runs after the animation finishes (e.g. after ~300ms, together with `setShouldRender(false)`).
2. Ensure that body scroll is still restored if the component unmounts while the slide-in is open. The simplest way is to add a separate `useEffect` with an empty dependency array that, in its cleanup, sets `document.body.style.overflow = ""`. This guarantees cleanup on unmount, independent of the `isOpen` animation timing.
</issue_to_address>
### Comment 2
<location> `turboui/src/SlideIn/index.tsx:127` </location>
<code_context>
+ );
+
+ // Use a portal to render the slide-in at the end of the document body
+ return createPortal(slideInContent, document.body);
+}
+
</code_context>
<issue_to_address>
**issue:** Accessing `document.body` directly can cause issues in non-DOM or SSR environments.
In SSR or non-browser environments, `document` is undefined and `createPortal(slideInContent, document.body)` will throw. Consider guarding with `typeof document !== "undefined"` and returning `null` or a non-portal fallback when the DOM is unavailable so the component is safe in isomorphic setups.
</issue_to_address>
### Comment 3
<location> `turboui/src/SlideIn/index.tsx:31` </location>
<code_context>
+ header,
+ testId,
+}: SlideInProps) {
+ const [shouldRender, setShouldRender] = useState(false);
+ const [isAnimating, setIsAnimating] = useState(false);
+ const slideInRef = useRef<HTMLDivElement>(null);
</code_context>
<issue_to_address>
**issue (complexity):** Consider simplifying the SlideIn component’s lifecycle by driving animations directly from `isOpen`, unmounting on `transitionend`, and extracting scroll-lock/ESC logic into separate hooks for clearer responsibilities.
The main complexity comes from the dual animation state + timers and the “kitchen sink” effect. You can keep all functionality while simplifying by:
1. **Use `isOpen` directly for animation, remove timers and `isAnimating`.**
2. **Keep `shouldRender` only for unmount-after-close, and use `transitionend` instead of a timeout.**
3. **Split the effect into focused hooks/effects for scroll lock and ESC handling.**
### 1. Simplify animation lifecycle (no timers, no `isAnimating`)
You can drive the animation purely from `isOpen` and use `transitionend` to know when to unmount:
```tsx
const [shouldRender, setShouldRender] = useState(false);
const slideInRef = useRef<HTMLDivElement>(null);
useEffect(() => {
if (isOpen) {
setShouldRender(true);
return;
}
// When closing, wait for the panel transition to finish, then unmount
const node = slideInRef.current;
if (!node) {
setShouldRender(false);
return;
}
const handleTransitionEnd = (event: TransitionEvent) => {
if (event.propertyName === "transform") {
setShouldRender(false);
}
};
node.addEventListener("transitionend", handleTransitionEnd);
return () => node.removeEventListener("transitionend", handleTransitionEnd);
}, [isOpen]);
```
Then in your JSX, use `isOpen` directly instead of `isAnimating` and drop the timers:
```tsx
if (!shouldRender) return null;
const slideInContent = (
<div
className={`fixed inset-0 z-50 flex justify-end ${
isOpen ? "bg-black/30" : "bg-black/0"
} transition-colors duration-300 ease-in-out`}
onClick={handleBackdropClick}
aria-modal="true"
role="dialog"
data-test-id={testId}
>
<div
ref={slideInRef}
className={`bg-surface-base shadow-2xl h-full overflow-auto flex flex-col ${
isOpen ? "translate-x-0" : "translate-x-full"
} transition-transform duration-300 ease-in-out ${contentClassName}`}
style={{ width }}
onClick={(e) => e.stopPropagation()}
>
{/* ... */}
</div>
</div>
);
```
This removes `isAnimating`, both timeouts, and makes the lifecycle:
`isOpen true → mount + open classes`
`isOpen false → close classes → transitionend → unmount`.
### 2. Split side effects: scroll lock and ESC into separate hooks/effects
Instead of handling body scroll and ESC inside the animation effect, move them to dedicated hooks/effects to make the component easier to scan:
```tsx
function useBodyScrollLock(active: boolean) {
useEffect(() => {
if (!active) return;
const prev = document.body.style.overflow;
document.body.style.overflow = "hidden";
return () => {
document.body.style.overflow = prev;
};
}, [active]);
}
function useEscToClose(active: boolean, onClose: () => void) {
useEffect(() => {
if (!active) return;
const handleEsc = (event: KeyboardEvent) => {
if (event.key === "Escape" && !event.defaultPrevented) {
onClose();
}
};
document.addEventListener("keydown", handleEsc);
return () => document.removeEventListener("keydown", handleEsc);
}, [active, onClose]);
}
```
Use them in `SlideIn`:
```tsx
useBodyScrollLock(isOpen);
useEscToClose(isOpen, onClose);
```
With these changes:
- Animation concerns are limited to `isOpen`, `shouldRender`, and one effect.
- Scroll lock and ESC handling are isolated, reusable, and easier to reason about.
- All existing behavior (open/close animation, backdrop, ESC close, scroll lock, unmount on close) is preserved.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
| ); | ||
|
|
||
| // Use a portal to render the slide-in at the end of the document body | ||
| return createPortal(slideInContent, document.body); |
Contributor
There was a problem hiding this comment.
issue: Accessing document.body directly can cause issues in non-DOM or SSR environments.
In SSR or non-browser environments, document is undefined and createPortal(slideInContent, document.body) will throw. Consider guarding with typeof document !== "undefined" and returning null or a non-portal fallback when the DOM is unavailable so the component is safe in isomorphic setups.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
No description provided.