Skip to content
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

Add move-in animation #511

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

Add move-in animation #511

wants to merge 10 commits into from

Conversation

byOsta
Copy link

@byOsta byOsta commented Mar 23, 2023

Add following animations:

  • Move in out
  • Move in
  • Move out
  • Fade in
  • Fade out

The move animations are a copy from the bootstrap ones.

Also add compatibility with animations on CustomLayouts if needed by adding the OverlayAnimationClass and ModalAnimationClass

e74640a6cb22bc29d24cd4ed628a37a4.mp4

@chrissainty
Copy link
Member

@byOsta I appreciate this has been almost a year, but if you would be happy to resolve the conflicts we can get this merged.

@chrissainty chrissainty added the Feature New feature that will be added to the project label Feb 19, 2024
@byOsta
Copy link
Author

byOsta commented Feb 20, 2024

@byOsta I appreciate this has been almost a year, but if you would be happy to resolve the conflicts we can get this merged.

Resolved conflicts @chrissainty :).

This PR also fixes @0ptim PRs #558 by setting the default ModalAnimationType for the custom layout modal as ModalAnimationType.None.

Comment on lines +3 to +7
<div>
<CascadingValue Value="this" IsFixed="true">
@Content
</CascadingValue>
</div>
Copy link
Member

Choose a reason for hiding this comment

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

Why is the div tag needed?

Copy link
Member

Choose a reason for hiding this comment

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

Removing it doesn't seem to effect anything

Copy link
Author

Choose a reason for hiding this comment

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

This is needed for the css to be applied to CustomLayouts.

Copy link
Member

@chrissainty chrissainty Feb 20, 2024

Choose a reason for hiding this comment

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

In what way? Just when it's removed everything looks to continue working as it did before and there doesn't seem to be any references to this element in the CSS, unless I've missed it?

Copy link
Author

@byOsta byOsta Feb 23, 2024

Choose a reason for hiding this comment

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

About the div...

This is without the div:
https://github.com/Blazored/Modal/assets/8792259/b1a629dd-fad1-4f51-9284-cc8765527ad1

This is with the div
https://github.com/Blazored/Modal/assets/8792259/4d12827a-e820-425f-bb6d-7191d8bcb945

Why does this happen?
Because of the Blazor CSS isolation
Without this change the custom layout that we have for the modals wont have the custom BlazoredModalInstance.razor.css styles applied...
I also had to add the following selectors on the BlazoredModalInstance.razor.css:
image

@chrissainty
Copy link
Member

While testing I noticed that the move out animation doesn't seem to work. The move-in part works fine, but when dismissing the modal it just fades out a bit rather than sliding up.

@byOsta
Copy link
Author

byOsta commented Feb 20, 2024

While testing I noticed that the move out animation doesn't seem to work. The move-in part works fine, but when dismissing the modal it just fades out a bit rather than sliding up.

Regarding this, yeah I noticed that too. I'm trying to debug it because it was working with the PR that I raised a year ago jajaja

@chrissainty
Copy link
Member

Whatever has changed has effected all the other animations as well, they all just fade out now rather than animate out.

src/Blazored.Modal/BlazoredModalInstance.razor.cs Outdated Show resolved Hide resolved
src/Blazored.Modal/BlazoredModalInstance.razor.cs Outdated Show resolved Hide resolved
src/Blazored.Modal/BlazoredModalInstance.razor.cs Outdated Show resolved Hide resolved
@byOsta
Copy link
Author

byOsta commented Feb 20, 2024

Whatever has changed has effected all the other animations as well, they all just fade out now rather than animate out.

After reviewing what breaks it is this commit
Specifically this: protected override bool ShouldRender() => false;...

@byOsta
Copy link
Author

byOsta commented Feb 20, 2024

Whatever has changed has effected all the other animations as well, they all just fade out now rather than animate out.

With this commit it's fixed.
The only comment not resolved it's the one related to the <div></div> and as I said it's needed to make the styles work with the custom layouts... If you remove them and go to the Custom Layout>Show Animated Modal on the samples you'll see that the animation stopped working.

src/Blazored.Modal/FocusTrap.razor Outdated Show resolved Hide resolved
Comment on lines +3 to +7
<div>
<CascadingValue Value="this" IsFixed="true">
@Content
</CascadingValue>
</div>
Copy link
Member

@chrissainty chrissainty Feb 20, 2024

Choose a reason for hiding this comment

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

In what way? Just when it's removed everything looks to continue working as it did before and there doesn't seem to be any references to this element in the CSS, unless I've missed it?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Feature New feature that will be added to the project
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants