771-feat: Mentorship post release updates: Modal improvements #773
Conversation
github-actions
bot
changed the title
|
Lighthouse Report:
|
|
Lighthouse Report:
|
src/entities/mentor/ui/mentor-feedback-card/mentor-feedback-card.test.tsx
Show resolved
Hide resolved
Quiddlee
changed the title
SpaNb4
requested review from
andron13,
dzmitry-varabei,
ansivgit and
natanchik
as code owners
📝 WalkthroughWalkthroughThe changes introduce two new easing function variables in the SCSS file and update test files to utilize the asynchronous Changes
Sequence Diagram(s)sequenceDiagram
participant U as User
participant M as Modal Component
participant D as DOM/Animation Handler
U->>M: Clicks close button
M->>M: Add 'fade-out' class
M->>D: Wait for 'animationend' event
D-->>M: Emits 'animationend'
M->>M: Remove 'fade-out' class and close modal
M->>onClose: Invoke onClose callback
Suggested labels
Suggested reviewers
✨ Finishing Touches
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
Documentation and Community
|
|
Lighthouse Report:
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
src/shared/ui/modal/modal.tsx (3)
22-22: Consider using ResizeObserver for dynamic scrollbar width updates.The scrollbar width calculation is only done once on mount. This might not handle dynamic changes to the scrollbar width (e.g., when content changes affect scrollbar visibility).
useEffect(() => { - scrollbarWidth.current = window.innerWidth - document.documentElement.clientWidth; + const updateWidth = () => { + scrollbarWidth.current = window.innerWidth - document.documentElement.clientWidth; + }; + const resizeObserver = new ResizeObserver(updateWidth); + resizeObserver.observe(document.documentElement); + return () => resizeObserver.disconnect(); }, []);Also applies to: 25-27, 86-88
29-37: Consider extracting animation duration from CSS.The animation duration is hardcoded in CSS. Consider making it configurable to ensure the cleanup timeout matches the animation duration.
+const FADE_OUT_DURATION = 300; // ms + const handleClose = useCallback(() => { dialogRef.current?.classList.add(cx('fade-out')); - dialogRef.current?.addEventListener('animationend', () => { + const cleanup = () => { dialogRef.current?.classList.remove(cx('fade-out')); dialogRef.current?.close(); onClose(); updateScrollbarWidth(); - }, { once: true }); + }; + dialogRef.current?.addEventListener('animationend', cleanup, { once: true }); + // Fallback cleanup in case animation event doesn't fire + setTimeout(cleanup, FADE_OUT_DURATION); }, [onClose]);
76-84: Consider adding aria attributes for better accessibility.The modal implementation could benefit from additional ARIA attributes.
<dialog className={cx('modal', className)} ref={dialogRef} onClose={handleClose} onKeyDown={handleCloseOnEscPress} data-testid="modal" + aria-modal="true" + aria-labelledby={title ? 'modal-title' : undefined} + role="dialog" >Also applies to: 99-99
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
src/shared/constants.ts(1 hunks)src/shared/ui/modal/modal.tsx(5 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: CI
🔇 Additional comments (3)
src/shared/constants.ts (1)
28-28: LGTM! Good use of type-safe constants.The
KEY_CODESconstant follows best practices by using uppercase naming and theas constassertion.src/shared/ui/modal/modal.tsx (2)
1-1: LGTM! Clean import organization.Good separation of React imports, third-party imports, and local imports.
Also applies to: 7-7
60-66: LGTM! Good keyboard accessibility implementation.The ESC key handler improves accessibility and follows best practices by using a constant for the key code.
|
Lighthouse Report:
|
| } | ||
|
|
||
| @include media-tablet { | ||
| padding: 0 16px 16px 16px; | ||
| width: 100dvw; | ||
| max-width: unset !important; |
There was a problem hiding this comment.
What about increasing specificity by using .dialog.modal instead of using !important?
There was a problem hiding this comment.
The animation seems too fast or something like that. Personally, I don't really like it
What type of PR is this? (select all that apply)
Description
Related Tickets & Documents
Screenshots, Recordings
Added/updated tests?
[optional] Are there any post deployment tasks we need to perform?
[optional] What gif best describes this PR or how it makes you feel?
Summary by CodeRabbit
New Features
Style
Bug Fixes