Skip to content

feat: full size modal #2620

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 35 commits into from
May 5, 2025
Merged

feat: full size modal #2620

merged 35 commits into from
May 5, 2025

Conversation

tewarig
Copy link
Contributor

@tewarig tewarig commented Apr 23, 2025

Description

Changes

Additional Information

Component Checklist

  • Update Component Status Page
  • Perform Manual Testing in Other Browsers
  • Add KitchenSink Story
  • Add Interaction Tests (if applicable)
  • Add changeset

Copy link

changeset-bot bot commented Apr 23, 2025

🦋 Changeset detected

Latest commit: 3bc0e8c

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 1 package
Name Type
@razorpay/blade Minor

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

Copy link
Contributor

github-actions bot commented Apr 23, 2025

✅ PR title follows Conventional Commits specification.

Copy link

codesandbox-ci bot commented Apr 23, 2025

This pull request is automatically built and testable in CodeSandbox.

To see build info of the built libraries, click here or the icon next to each commit SHA.

Latest deployment of this branch, based on commit 3bc0e8c:

Sandbox Source
razorpay/blade: basic Configuration

@rzpcibot
Copy link
Collaborator

rzpcibot commented Apr 23, 2025

Bundle Size Report

Updated Components
Status Component Base Size (kb) Current Size (kb) Diff
Modal 13.267 13.337 +0.070 KB

Generated by 🚫 dangerJS against 3bc0e8c

Copy link
Member

Choose a reason for hiding this comment

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

why separate story file?

Copy link
Member

Choose a reason for hiding this comment

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

I think here you can just do

const FullScreenModal = ModalTemplate.bind({})
FullScreenModal.args = {
  size: 'full'
}

This should do the job right?

Copy link
Member

Choose a reason for hiding this comment

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

We can have example with StepGroup in our CreationView documentation I think. Otherwise at multiple places we're end up keeping very similar examples and larger examples in stories can make the "show code" section a bit confusing

`;
const ModalContent = styled(BaseBox)<{ isVisible: boolean; size: NonNullable<ModalProps['size']> }>(
({ isVisible, theme, size }) => {
const centerTransform = size === 'full' ? 'none' : 'translate(-50%, -50%)';
Copy link
Member

Choose a reason for hiding this comment

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

Is this animation defined from design?

Copy link
Contributor Author

@tewarig tewarig Apr 25, 2025

Choose a reason for hiding this comment

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

nope. i have kept is same for normal and full size modal.

Screen.Recording.2025-04-25.at.12.45.15.PM.mov

but we don't need translate(-50%, -50%) .. in case of full page modal. since it will already at center.
also had a discussion with RK. will change it to just fadein and fadeout

Comment on lines 172 to 174
? `calc(100vw - ${makeSize(modalMargin[size])} - ${makeSize(
modalMargin[size],
)})`
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
? `calc(100vw - ${makeSize(modalMargin[size])} - ${makeSize(
modalMargin[size],
)})`
? `calc(100vw - ${makeSize(modalMargin[size] * 2)},
)})`

Same thing?

Comment on lines 181 to 182
top={size === 'full' ? makeSize(modalMargin[size]) : '50%'}
left={size === 'full' ? makeSize(modalMargin[size]) : '50%'}
Copy link
Member

Choose a reason for hiding this comment

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

Why do we have to define different value for full? wouldn't it still be center aligned just full width?

} & DataAnalyticsAttribute;

const _ModalBody = ({
children,
padding = 'spacing.6',
height,
Copy link
Member

Choose a reason for hiding this comment

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

should ModalBody have height prop or should Modal component have height prop?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The Modal body should have a height prop, as in the case of a full Modal, we need the Modal body to take up 100% of the available height inside the Modal.

Copy link
Member

@saurabhdaware saurabhdaware Apr 30, 2025

Choose a reason for hiding this comment

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

Ohhh what happens if I do size="full" and not define height here?

Can we not define height="100%" for all ModalBody without taking it from consumers?

Copy link
Member

Choose a reason for hiding this comment

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

We can have example with StepGroup in our CreationView documentation I think. Otherwise at multiple places we're end up keeping very similar examples and larger examples in stories can make the "show code" section a bit confusing

@saurabhdaware
Copy link
Member

image

borderRadius on bottom left is not working 🤔

Comment on lines 34 to 48
const entry = keyframes`
from {
opacity: 0;
transform: translate(-50%, -50%) scale(0.9) translateY(20px);
}
to {
opacity: 1;
transform: translate(-50%, -50%) scale(1) translateY(0px);
}
`;
const ModalContent = styled(BaseBox)<{ isVisible: boolean; size: NonNullable<ModalProps['size']> }>(
({ isVisible, theme, size }) => {
const entry = keyframes`
from {
opacity: 0;
transform: translate(-50%, -50%) scale(0.9) translateY(20px);
}
to {
opacity: 1;
transform: translate(-50%, -50%) scale(1) translateY(0px);
}
`;

Copy link
Member

Choose a reason for hiding this comment

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

This change is not needed anymore right? we can move keyframes outside and revert to previous state

@saurabhdaware
Copy link
Member

Few comments. Rest looks good:

animation-flash.mov

@tewarig
Copy link
Contributor Author

tewarig commented Apr 30, 2025

Few comments. Rest looks good:

animation-flash.mov

found the issue

Screen.Recording.2025-04-30.at.5.44.09.PM.mov

@tewarig
Copy link
Contributor Author

tewarig commented Apr 30, 2025

chore: update css

seems like same issue exists in master in case of nomal modal

modal-issue-master.mov
Screenshot 2025-04-30 at 8 44 16 PM

@tewarig
Copy link
Contributor Author

tewarig commented May 2, 2025

@saurabhdaware let me know if u can pick this animation bug in another pr ?

import { FloatingFocusManager, FloatingPortal, useFloating } from '@floating-ui/react';
import usePresence from 'use-presence';
import { m } from 'framer-motion';
Copy link
Member

Choose a reason for hiding this comment

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

nice

Copy link
Member

Choose a reason for hiding this comment

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

Our modal seems to lag a bit too much now with this though. Its a bit too visible 😭 . Lets go back to previous CSS animation and lets live with that flash of content instead?

Screen.Recording.2025-05-05.at.11.03.00.AM.mov

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@saurabhdaware reverted changes

@tewarig tewarig merged commit f7b868a into master May 5, 2025
13 of 15 checks passed
@tewarig tewarig deleted the feat/full-size-modal branch May 5, 2025 11:58
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants