Bugfix: modal wobble on transition in/out

[Mahmoud-zino]

Linked Issue

Closes #2239
Closes #1870

Description

Fixed the issue by checking the client height and comparing it to the window height and then adding the overflow based on the result.

Note: cOverflow is added directly to the classes of the backdrop because otherwise the reactivity won't be triggered on every change.

Changsets

Instructions: Changesets automate our changelog. If you modify files in /packages, run pnpm changeset in the root of the monorepo, follow the prompts, then commit the markdown file. Changes that add features should be minor while chores and bugfixes should be patch. Please prefix the changeset message with feat:, bugfix: or chore:.

Checklist

Please read and apply all contribution requirements.

• This PR targets the dev branch (NEVER master)
• Documentation reflects all relevant changes
• Branch is prefixed with: docs/, feat/, chore/, bugfix/
• Ensure Svelte and Typescript linting is current - run pnpm ci:check
• Ensure Prettier linting is current - run pnpm format
• All test cases are passing - run pnpm test
• Includes a changeset (if relevant; see above)

[changeset-bot]

🦋 Changeset detected

Latest commit: 864066c

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

This PR includes changesets to release 1 package

Name	Type
@skeletonlabs/skeleton	Patch

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

[vercel]

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

Name	Status	Preview	Updated (UTC)
skeleton-docs	✅ Ready (Inspect)	Visit Preview	Dec 19, 2023 9:03pm

[Mahmoud-zino]

@endigo9740 this won't solve #1870, but I though separating them will simplify the review.
If this solution is accepted we can close #2241 . Thank you @CoreyT355 for helping with this bug.

[CoreyT355]

@endigo9740 this won't solve #1870, but I though separating them will simplify the review. If this solution is accepted we can close #2241 . Thank you @CoreyT355 for helping with this bug.

@Mahmoud-zino That's a nice solution. I haven't worked with the svelte:window piece yet, so I didn't think about it when the idea of catching the window height came up.
Thanks for finding a solution here. Hopefully this gets merged in.

[endigo9740]

@CoreyT355 @Mahmoud-zino I've been handling PR reviews on Sunday's (tomorrow for me) so expect a proper audit then.

[endigo9740]

@Mahmoud-zino just a couple small nitpicks here, but overall I like this. It seems to be a great solution to the issue at hand.

[endigo9740]

@Mahmoud-zino unfortunately we seem to have reverted back to the previous issue where portions of the overflowing modal get cut off. He's an example displayed on production:

And here's what this looks like per your feature branch:

I'll have more bandwidth come December 18th, so feel free to tinker with this until then. Then I'll plan to do a proper sit down and review of this feature top to bottom to see if I can determine a definitive fix for all sizing. Again we need to support:

• Modals that are smaller than the canvas
• Modals that are exactly 1:1 with the canvas
• Modals that are taller than the canvas (enables scrolling)

It's definitely a tough nut to crack!

[Mahmoud-zino]

@endigo9740 I just checked it again, this is the effect of having max-h-screen, deleting it solved the issue.

[endigo9740]

@Mahmoud-zino That's awesome. In order to properly test this, can you do me a favor and implement a new component modal example that is full screen? Just follow the lead of the current examples. Without that we don't have a great way to test all three conditions I mentioned above. The contents of the modal can be anything - even just placeholder text that says: "Full Screen Example". I'll polish it a bit more later.

[endigo9740]

@Mahmoud-zino I'll set this back to the "draft" state until the new demo is ready. That'll be the last test before we merge. Unfortunately this will miss today's release, but we'll aim to get it in the next!

[Mahmoud-zino]

@endigo9740 sorry for missing the release, I added an example of the full-screen modal with lorem but feel free to change it if needed.
Also I noticed the X button still have the c transition but I am not sure if this is acceptable or not, please let me know if not (I will have to investigate further).

[endigo9740]

Nah, I meant to comment and say it'll be better for me to review from this weekend forward. I won't be juggling my current full time commitments on top of all this. Should given me more time to test things thoroughly and ensure we get this sorted once and for all.

[endigo9740]

@Mahmoud-zino I've moved, renamed, and updated the the new example "full" modal option. I've also update the contents of the modal to provide some simpler styles for showcasing how this can work.

NOTE: there is currently no way to disable the backdrop > modal-transition built in padding. But I think that's an issue we should address in Skeleton v3.

I also see how you're handling modals that are oversized vertically. The scrollbar won't appear as you resize your window, but will appear if that size is too large when the modal appears. I think that's a happy medium for now. Again, I'd like to tackle the root issues for this in v3.

Assuming you're happy with my changes, feel free to approve this PR and I'll aim to merge right away. Thanks for the hard work on this one!

[Mahmoud-zino]

@endigo9740 I can't approve my own PR, but it looks good to me 👍