Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
💡 Enable Vercel Agent with $100 free credit for automated AI reviews |
There was a problem hiding this comment.
Pull Request Overview
This pull request adds animated transition support and data attribute synchronization to popover components in both HTML and React packages. The changes implement a state machine for tracking transition phases, enable smooth entrance/exit animations, and ensure data attributes are properly shared between trigger and popup elements.
Key Changes:
- Introduced transition status tracking (
initial,open,close,unmounted) for controlling popover animation states - Added CSS transitions and data attribute-based styling for scale and opacity animations during open/close
- Implemented data attribute synchronization from trigger to popup elements for consistent state representation
Reviewed Changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 4 comments.
| File | Description |
|---|---|
| packages/html/html/src/components/media-popover.ts | Added transition status state machine, data attribute observer, and attribute synchronization between trigger and popup |
| packages/html/html/src/skins/media-skin-default.ts | Added CSS transitions and styling for popover animations based on data attributes |
| packages/react/react/src/components/Popover.tsx | Integrated useTransitionStatus hook, removed conditional rendering, added data attribute copying and transition status attributes |
| packages/react/react/src/skins/frosted/styles.ts | Added Tailwind classes for transition animations with data attribute selectors |
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
| const mutationObserver = new MutationObserver((mutations) => { | ||
| mutations.forEach((mutation) => { | ||
| if (mutation.type === 'attributes') { | ||
| const rootElement = this.closest('media-popover-root') as MediaPopoverRoot; | ||
| let popupElement = rootElement.querySelector('media-popover-popup') as MediaPopoverPopup; | ||
|
|
||
| if (!popupElement) { | ||
| const portalElement = rootElement.querySelector('media-popover-portal') as MediaPopoverPortal; | ||
| if (!portalElement) { | ||
| return; | ||
| } | ||
|
|
||
| popupElement = portalElement.querySelector('media-popover-popup') as MediaPopoverPopup; | ||
| if (!popupElement) { | ||
| return; | ||
| } | ||
| } | ||
|
|
||
| const attributeName = mutation.attributeName; | ||
| if (!attributeName || !attributeName.startsWith('data-')) { | ||
| return; | ||
| } | ||
|
|
||
| const attributeValue = triggerElement.getAttribute(attributeName); | ||
| if (attributeValue !== null) { | ||
| popupElement.setAttribute(attributeName, attributeValue); | ||
| } else { | ||
| popupElement.removeAttribute(attributeName); | ||
| } | ||
| } | ||
| }); | ||
| }); |
There was a problem hiding this comment.
The MutationObserver performs repeated DOM queries for popup elements on every attribute mutation. Consider caching the popup element reference or querying it once during initialization to avoid unnecessary DOM traversals on each mutation.
| mutationObserver.observe(triggerElement, { | ||
| attributes: true, | ||
| }); |
There was a problem hiding this comment.
The MutationObserver is created but never cleaned up. Store the observer instance and call disconnect() in disconnectedCallback() to prevent memory leaks.
| // Copy data attributes from trigger element | ||
| const dataAttributes = triggerElement?.attributes | ||
| ? Object.fromEntries( | ||
| Array.from(triggerElement.attributes) | ||
| .filter(attr => attr.name.startsWith('data-')) | ||
| .map(attr => [attr.name, attr.value]), | ||
| ) | ||
| : {}; |
There was a problem hiding this comment.
Data attributes are extracted from the trigger element on every render. Consider memoizing this computation with useMemo() to avoid unnecessary Array operations and object creation when triggerElement hasn't changed.
| // Copy data attributes from trigger element | |
| const dataAttributes = triggerElement?.attributes | |
| ? Object.fromEntries( | |
| Array.from(triggerElement.attributes) | |
| .filter(attr => attr.name.startsWith('data-')) | |
| .map(attr => [attr.name, attr.value]), | |
| ) | |
| : {}; | |
| // Copy data attributes from trigger element, memoized | |
| const dataAttributes = useMemo(() => { | |
| return triggerElement?.attributes | |
| ? Object.fromEntries( | |
| Array.from(triggerElement.attributes) | |
| .filter(attr => attr.name.startsWith('data-')) | |
| .map(attr => [attr.name, attr.value]), | |
| ) | |
| : {}; | |
| }, [triggerElement]); |
| transform: scale(0.9) translateY(8px); | ||
| opacity: 0; | ||
| } | ||
There was a problem hiding this comment.
Remove trailing whitespace on this empty line.
This pull request enhances the popover components in both the HTML and React packages to support animated transitions and improved data attribute synchronization between triggers and popups. The changes introduce a transition status state machine, update styles for smooth entrance and exit animations, and ensure that accessibility and custom data attributes are correctly propagated. These improvements make popover interactions more visually appealing and robust.
Popover Transition State and Animation Enhancements
#transitionStatusstate toMediaPopoverRootand synchronized its value with popover visibility, enabling fine-grained control over popover transitions (media-popover.ts). [1] [2] [3]media-skin-default.ts,frosted/styles.ts). [1] [2]Data Attribute Synchronization and Accessibility
MediaPopoverTriggerto observe and copy data attributes from the trigger element to the popup, ensuring custom states and accessibility attributes are reflected in the popup (media-popover.ts).data-attributes from the trigger element to the popup, and exposedtransitionStatusin context for use in popup rendering (Popover.tsx). [1] [2]Popover API and Behavior Improvements (React)
useTransitionStatusto track and provide transition status, removed conditional rendering of the popup based onopenstate, and updated context and trigger handling for better state management (Popover.tsx). [1] [2] [3] [4]These changes collectively improve the user experience and maintainability of popover components by enabling smooth transitions and consistent attribute propagation.