Skip to content

fix(modal): change default visibility to original footer when expandToScroll is false #30310

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

Open
wants to merge 9 commits into
base: main
Choose a base branch
from

Conversation

kumibrr
Copy link
Contributor

@kumibrr kumibrr commented Mar 26, 2025

Issue number: resolves #30315


What is the current behavior?

When expandToScroll = false, the original footer is swapped by a cloned one to avoid animation issues.
But if the original footer has some eventListeners, they won't work on the cloned footer.

What is the new behavior?

Footer visibility is the original by default, and the visibility will swap to the cloned one when the animation starts, and will be swapped back to the original when the animation ends

Does this introduce a breaking change?

  • Yes
  • No

Other information

Before After
Screen.Recording.2025-03-26.at.19.13.47.mov
Screen.Recording.2025-03-26.at.19.14.32.mov

EDIT: The button in the videos "jump" from left to right, this is caused by another issue I discovered yesterday (stylesheets not being applied), but it's quite tricky to solve. I've opened an issue for that #30312

@kumibrr kumibrr requested a review from a team as a code owner March 26, 2025 18:17
@kumibrr kumibrr requested a review from brandyscarney March 26, 2025 18:17
Copy link

vercel bot commented Mar 26, 2025

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Comments Updated (UTC)
ionic-framework ✅ Ready (Inspect) Visit Preview 💬 Add feedback Apr 4, 2025 6:21pm

@github-actions github-actions bot added the package: core @ionic/core package label Mar 26, 2025
@kumibrr kumibrr changed the title fix(sheet): change visibility to original footer when expandToScroll is false fix(sheet): change default visibility to original footer when expandToScroll is false Mar 26, 2025
@thetaPC
Copy link
Contributor

thetaPC commented Mar 26, 2025

Thank you for submitting the PR! However, we require issues to be submitted first and then tied to the PR for us to proceed with reviewing.

@kumibrr
Copy link
Contributor Author

kumibrr commented Mar 26, 2025

Thank you for submitting the PR! However, we require issues to be submitted first and then tied to the PR for us to proceed with reviewing.

Done!

Copy link
Contributor

@thetaPC thetaPC left a comment

Choose a reason for hiding this comment

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

Let's also add a test to prevent a regression under modal/test/sheet/modal.e2e.ts. I recommend using this test as a base. Place the new test under the rendering group.

@kumibrr
Copy link
Contributor Author

kumibrr commented Apr 4, 2025

@thetaPC Done!

Copy link
Contributor

@thetaPC thetaPC left a comment

Choose a reason for hiding this comment

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

Almost, just some minor requests!

kumibrr and others added 3 commits April 4, 2025 20:17
Co-authored-by: Maria Hutt <[email protected]>
Co-authored-by: Maria Hutt <[email protected]>
Co-authored-by: Maria Hutt <[email protected]>
Copy link
Contributor

@thetaPC thetaPC left a comment

Choose a reason for hiding this comment

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

LGTM, requesting more approvals

@thetaPC thetaPC requested a review from ShaneK April 4, 2025 21:39
@brandyscarney brandyscarney changed the title fix(sheet): change default visibility to original footer when expandToScroll is false fix(modal): change default visibility to original footer when expandToScroll is false Apr 7, 2025
Copy link
Member

@brandyscarney brandyscarney left a comment

Choose a reason for hiding this comment

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

Thank you for the PR and your continued work on this, we really appreciate it. 🙂

I noticed an issue on md mode that was introduced with this fix. The footer is not visible and then becomes visible after the modal is fully presented. Here are comparison videos of the modal on main vs this PR:

main PR branch
main-sheet.mp4
PR-sheet.mp4

This can be reproduced on Chrome, Firefox and Safari without emulating a device. It does not happen in ios mode.

@kumibrr
Copy link
Contributor Author

kumibrr commented Apr 9, 2025

@brandyscarney Thank you for pointing that issue. I'll work on it later this weekend.

@thetaPC
Copy link
Contributor

thetaPC commented Apr 9, 2025

@kumibrr Let's place this PR on hold. Due to the increase number of issues submitted due to the ion-footer, we are going go back to the drawing board on how we can render the original footer instead of making a copy.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
package: core @ionic/core package
Projects
None yet
Development

Successfully merging this pull request may close these issues.

bug: sheet modal. EventListeners don't work on ion-footer when expandToScroll is false
3 participants