Conversation
There was a problem hiding this comment.
Pull request overview
Adds fade/scale animations to popup modals/dropdowns to replace prior height/slide transitions.
Changes:
- Reworks
AccountHeaderModalopen/close behavior frommax-heightanimation to opacity/scale with an--openmodifier class. - Updates
SlideupModalanimation from translateY slide to opacity/scale, and adjusts dismiss timeout to 150ms. - Makes
AccountHeaderModal__contentscrollable viaoverflow: auto.
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 5 comments.
| File | Description |
|---|---|
| extension/src/popup/components/account/AccountHeaderModal/styles.scss | Implements fade/scale animation + adds scroll overflow on modal content. |
| extension/src/popup/components/account/AccountHeaderModal/index.tsx | Switches to class-based open state (removes ref/effect max-height control). |
| extension/src/popup/components/SlideupModal/styles.scss | Changes modal animation to fade/scale with 150ms transitions. |
| extension/src/popup/components/SlideupModal/index.tsx | Updates dismiss timeout/comment to match new 150ms transition duration. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| /* Fade animation */ | ||
| opacity: 0; | ||
| transform: scale(0.95); | ||
| pointer-events: none; | ||
| transition: | ||
| opacity 0.15s cubic-bezier(0.16, 1, 0.3, 1), | ||
| transform 0.15s cubic-bezier(0.16, 1, 0.3, 1); | ||
|
|
There was a problem hiding this comment.
With only opacity: 0/pointer-events: none, the dropdown remains focusable to keyboard and present to assistive tech when closed (e.g., the hidden items can still be tabbed to). Consider also toggling visibility: hidden (and/or applying inert/aria-hidden from the component) when closed so the content is removed from the tab order and accessibility tree.
| opacity 0.15s cubic-bezier(0.16, 1, 0.3, 1), | ||
| transform 0.15s cubic-bezier(0.16, 1, 0.3, 1); |
There was a problem hiding this comment.
This file previously used the shared --dropdown-animation timing, but the new transition hard-codes 0.15s and a custom easing. To keep animation timings consistent and avoid future drift, consider using var(--dropdown-animation) (defined in popup/styles/global.scss) or introducing a new shared CSS variable for modal fade transitions.
| opacity 0.15s cubic-bezier(0.16, 1, 0.3, 1), | |
| transform 0.15s cubic-bezier(0.16, 1, 0.3, 1); | |
| opacity var(--dropdown-animation), | |
| transform var(--dropdown-animation); |
| <div | ||
| className={`AccountHeaderModal ${isDropdownOpen ? "AccountHeaderModal--open" : ""} ${className || ""}`} | ||
| > | ||
| <div className="AccountHeaderModal__content">{children}</div> |
There was a problem hiding this comment.
The modal stays mounted even when closed; adding/removing a CSS class won’t prevent keyboard focus on descendants (links/buttons inside children). Consider adding aria-hidden={!isDropdownOpen} and/or inert when closed (or unmounting after the fade-out) so hidden menu items aren’t reachable via Tab / screen readers.
| <div className="AccountHeaderModal__content">{children}</div> | |
| <div | |
| className="AccountHeaderModal__content" | |
| aria-hidden={!isDropdownOpen} | |
| inert={!isDropdownOpen ? "" : undefined} | |
| > | |
| {children} | |
| </div> |
| setIsOpen(false); | ||
| // our dismiss transition is 200ms long | ||
| // our dismiss transition is 150ms long | ||
| setTimeout(() => { | ||
| setIsModalOpen(false); | ||
| }, 200); | ||
| }, 150); | ||
| }} |
There was a problem hiding this comment.
The close timeout is hard-coded and now shorter than the LoadingBackground fade-out (transition: all var(--dropdown-animation) + transition-delay: 0.1s), so when the parent unmounts the modal after 150ms the backdrop transition is cut off mid-animation. Consider waiting for transitionend (modal and/or backdrop) before calling setIsModalOpen(false), or at least deriving a single shared duration (incl. delay) so the JS unmount timing can’t drift from CSS.
| transform: scale(0.98); | ||
| opacity: 0; | ||
| pointer-events: none; | ||
|
|
There was a problem hiding this comment.
When the modal is “closed”, it’s still in the DOM and opacity: 0/pointer-events: none won’t stop keyboard focus on any focusable descendants. Consider pairing the visual hide with visibility: hidden (or inert/aria-hidden at the component level) so users can’t tab into an invisible slideup modal.
|
@adxdits Thanks so much for the contribution! I will review this work with the design team this week and will work towards getting this included in the next release |
Great 👍 |
fix #2618