Skip to content

feat(composedmodal): L3-10803 style updates to composed modal pin footer at bottom and go fullscreen on mobile#770

Closed
scottdickerson wants to merge 10 commits intomainfrom
L3-10803-composed-modal-styles
Closed

feat(composedmodal): L3-10803 style updates to composed modal pin footer at bottom and go fullscreen on mobile#770
scottdickerson wants to merge 10 commits intomainfrom
L3-10803-composed-modal-styles

Conversation

@scottdickerson
Copy link
Contributor

@scottdickerson scottdickerson commented Nov 20, 2025

Jira ticket

L3-10803

Screenshots

Before After
image image

On Chromatic you'll see that now we have both desktop and mobile breakpoints for all the stories:
image

Figma link

Figma Link

Summary

This pull request introduces improvements to modal components and Storybook configuration, focusing on better responsive behavior and enhanced modal footer handling. The main changes include dynamic footer height measurement for ComposedModal, updated Storybook viewports for more accurate previews on mobile components, and improved styling for modals to support mobile and desktop layouts.

Modal Component Enhancements

  • Added dynamic measurement of the modal footer height in ComposedModal, using useRef, useState, and ResizeObserver to set a CSS variable for responsive layout adjustments. The footer is now rendered as a dedicated element with a reference. (src/components/ComposedModal/ComposedModal.tsx) [1] [2] [3] [4]
  • Updated modal and footer styles to use CSS variables for height, improved mobile responsiveness, and ensured the modal body and footer are laid out correctly across viewports. (src/components/ComposedModal/_composedModal.scss) [1] [2] [3] [4]

Storybook Configuration and Stories

  • Configured Storybook to support custom viewports for mobile, tablet, and desktop, set the default viewport to desktop, and added Chromatic visual regression viewports. (.storybook/preview.tsx)
  • Updated modal and composed modal stories to open modals by default for easier previewing, and added empty tags arrays for consistency. (src/components/ComposedModal/ComposedModal.stories.tsx, src/components/Modal/Modal.stories.tsx) [1] [2] [3] [4] [5]
  • Changed the mobile form story to use Storybook globals for viewport selection instead of parameters, aligning with the new configuration. (src/patterns/ProgressWizard/ProgressWizard.stories.tsx)

Minor Style Adjustments

  • Adjusted margin for the viewing details content for better visual spacing. (src/patterns/ViewingDetails/_viewingDetails.scss)

Acceptance Test (how to verify the PR)

  • Check the ComposedModal at mobile breakpoints

Regression Test

  • Regression test the other modals using ComposedModals
  • Confirm the differences in the storybook chromatic visual tests (FYI there will be 200 new stories because of running at the second viewport)

Evidence of testing

  • Post logs, screenshots, etc

Things to look for during review

  • PR title should correctly describe the most significant type of commit. I.e. feat(scope): ... if a minor release should be triggered.
  • All commit messages follow convention and are appropriate for the changes
  • All references to phillips class prefix are using the prefix variable
  • All major areas have a data-testid attribute.
  • Document all props with jsdoc comments
  • All strings should be translatable.
  • Unit tests should be written and should have a coverage of 90% or higher in all areas.

New Components

  • Are there any accessibility considerations that need to be taken into account and tested?
  • Default story called "Playground" should be created for all new components
  • Create a jsdoc comment that has an Overview section and a link to the Figma design for the component
  • Export the component and its typescript type from the index.ts file
  • Import the component scss file into the componentStyles.scss file.

@netlify
Copy link

netlify bot commented Nov 20, 2025

Deploy Preview for phillips-seldon ready!

Name Link
🔨 Latest commit 8d7a5f1
🔍 Latest deploy log https://app.netlify.com/projects/phillips-seldon/deploys/691f75e0ccd09300080b9ffb
😎 Deploy Preview https://deploy-preview-770--phillips-seldon.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify project configuration.

Copy link
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull Request Overview

This PR enhances the ComposedModal component to support a pinned footer at the bottom and full-screen behavior on mobile devices. The changes introduce dynamic footer height measurement using ResizeObserver and CSS custom properties to ensure proper layout across different viewports.

Key changes:

  • Dynamic footer height tracking in ComposedModal using refs, state, and ResizeObserver to calculate and apply footer positioning
  • Updated SCSS styles to position the footer absolutely at the bottom, adjust modal dimensions for mobile/desktop, and use CSS variables for responsive spacing
  • Storybook configuration updates including custom viewport definitions, default viewport settings, and Chromatic visual regression viewports

Reviewed Changes

Copilot reviewed 7 out of 7 changed files in this pull request and generated 7 comments.

Show a summary per file
File Description
src/components/ComposedModal/ComposedModal.tsx Added footer ref, height state, and ResizeObserver logic to dynamically measure footer height; applied CSS variable to modal style
src/components/ComposedModal/_composedModal.scss Refactored styles to use absolute positioning for footer, added mobile fullscreen layout, and applied CSS variables for dynamic height calculations
src/components/ComposedModal/ComposedModal.stories.tsx Changed default isOpen state to true in stories and added empty tags array
src/components/Modal/Modal.stories.tsx Changed default isOpen state to true in stories and added empty tags array
.storybook/preview.tsx Added custom viewport configurations, set default viewport to desktop, configured Chromatic viewports, and removed unused React import
src/patterns/ProgressWizard/ProgressWizard.stories.tsx Migrated viewport configuration from parameters to globals API
src/patterns/ViewingDetails/_viewingDetails.scss Simplified margin styling by removing margin-top and updating margin-bottom

return () => {
resizeObserver.disconnect();
};
}, [secondaryButton, primaryButton, footerContent, footerHeight]);
Copy link

Copilot AI Nov 20, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The footerHeight state variable is included in the useEffect dependency array, but it's updated within the effect's callback. This creates an unnecessary re-run of the effect whenever footerHeight changes. Since the ResizeObserver already handles dynamic measurements, footerHeight should be removed from the dependencies to avoid potential infinite loops or redundant observer setup/teardown cycles.

Suggested change
}, [secondaryButton, primaryButton, footerContent, footerHeight]);
}, [secondaryButton, primaryButton, footerContent]);

Copilot uses AI. Check for mistakes.
};

// Initial measurement
requestAnimationFrame(measureFooter);
Copy link

Copilot AI Nov 20, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The requestAnimationFrame call for initial measurement is not cancelled if the component unmounts before the frame executes. This could lead to a state update on an unmounted component. Consider storing the returned frame ID and cancelling it in the cleanup function using cancelAnimationFrame.

Copilot uses AI. Check for mistakes.
min-width: 0;
display: flex;
flex-direction: column;
height: min(fit-content, calc(100svh - (2 * var(--modal-footer-height, 0px))));
Copy link

Copilot AI Nov 20, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The calculation 100svh - (2 * var(--modal-footer-height, 0px)) subtracts twice the footer height, but the footer only appears once. This appears to be incorrect and may cause unexpected layout issues. The calculation should likely be 100svh - var(--modal-footer-height, 0px) unless there's a specific reason for doubling the footer height.

Suggested change
height: min(fit-content, calc(100svh - (2 * var(--modal-footer-height, 0px))));
height: min(fit-content, calc(100svh - var(--modal-footer-height, 0px)));

Copilot uses AI. Check for mistakes.
height: min(fit-content, calc(100svh - (2 * var(--modal-footer-height, 0px))));
max-height: 100svh;
min-width: 35rem;
overflow: auto;
Copy link

Copilot AI Nov 20, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Setting overflow: auto on the modal container combined with overflow-y: auto on the body (line 27) creates nested scrollable regions. This can lead to confusing scroll behavior for users. Consider managing overflow only at the body level to maintain a single, predictable scroll context.

Suggested change
overflow: auto;
/* Removed overflow: auto; to prevent nested scroll regions */

Copilot uses AI. Check for mistakes.
Comment on lines +41 to +47
&__footer {
background-color: $white-100;
bottom: 0;
padding-bottom: $spacing-md;
position: absolute;
width: 100%;
}
Copy link

Copilot AI Nov 20, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The absolutely positioned footer with width: 100% may overflow the modal container if the modal has padding or borders. Additionally, absolute positioning requires the parent to have position: relative set. The modal container (.#{$px}-composed-modal) should explicitly set position: relative to establish a positioning context for the footer.

Copilot uses AI. Check for mistakes.
margin: 0 $spacing-lg $spacing-md;
max-height: var('--max-modal-body-height');
max-height: min(fit-content, calc(100% - var(--modal-footer-height, 0px)));
overflow-y: auto;
Copy link

Copilot AI Nov 20, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[nitpick] Adding padding-bottom: var(--modal-footer-height) to create space for the absolutely positioned footer is correct, but this creates a dependency between the body padding and footer positioning. If the footer height changes before the CSS variable updates, content may be obscured. Consider documenting this relationship in a comment for future maintainers.

Suggested change
overflow-y: auto;
overflow-y: auto;
// NOTE: This padding-bottom must match the height of the absolutely positioned footer.
// If the footer height changes, ensure --modal-footer-height is updated to prevent content from being obscured.

Copilot uses AI. Check for mistakes.
Comment on lines +114 to +118
style={
{
'--modal-footer-height': footerHeight,
} as React.CSSProperties
}
Copy link

Copilot AI Nov 20, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The new dynamic footer height functionality using CSS custom properties lacks test coverage. The existing test file doesn't verify that the --modal-footer-height CSS variable is correctly set or that the ResizeObserver properly updates it. Add tests to ensure this critical responsive behavior works as expected.

Copilot generated this review using guidance from repository custom instructions.
@github-actions
Copy link

github-actions bot commented Nov 20, 2025

@scottdickerson scottdickerson changed the title feat(composedmodal): L3-10803 pin style updates to composed modal pin footer at bottom and go fullscreen on mobile feat(composedmodal): L3-10803 style updates to composed modal pin footer at bottom and go fullscreen on mobile Nov 20, 2025
@scottdickerson scottdickerson added the DO_NOT_MERGE Either in feature freeze mode or not ready for merge label Dec 3, 2025
@netlify
Copy link

netlify bot commented Jan 15, 2026

Deploy Preview for phillips-seldon ready!

Name Link
🔨 Latest commit b6adb86
🔍 Latest deploy log https://app.netlify.com/projects/phillips-seldon/deploys/696929bae775ce000858e2ff
😎 Deploy Preview https://deploy-preview-770--phillips-seldon.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify project configuration.

@smarks8
Copy link
Contributor

smarks8 commented Jan 15, 2026

looks similar to another open pr: L3-10837-add-min-widths-to-modals https://phillipsauctions.atlassian.net/browse/L3-10837

The figma linked in that ticket (which is from design) doesn't have the modal going full screen on mobile . It also has defined widths at each breakpoint


export const Playground = (props: ComposedModalProps) => {
const [isOpen, setIsOpen] = useState(false);
const [isOpen, setIsOpen] = useState(true);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This makes it easier to see per story, but it opens them all at the same time in the overview, not sure if that would cause issues in chromatic

border-radius: $radius-md;
max-width: 95vw;
min-width: 0;
display: flex;
Copy link
Contributor

@smarks8 smarks8 Jan 15, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we might want to use the changes in Aaron's pr for the modal

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

DO_NOT_MERGE Either in feature freeze mode or not ready for merge

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants