minikin / popover

Popover for Flutter. A popover is a transient view that appears above other content onscreen when you tap a control or in an area.
https://pub.dev/packages/popover
MIT License
166 stars 54 forks source link

Add Animation Transition #50

Closed sanjidbillah closed 2 years ago

sanjidbillah commented 2 years ago

I have added animation transition. ScaleTransition as default. If i need change anything let me know. fix for #48

minikin commented 2 years ago

@sanjidbillah awesome, thank you very much for your contribution! I'll check it in detail asap.

prateekmedia commented 2 years ago

@sanjidbillah Nice Try, but it doesn't work, when setting it to fade transition I am still getting the same:

https://user-images.githubusercontent.com/41370460/153723498-25531d82-1e14-40f5-a52d-24566e37508b.mp4

https://user-images.githubusercontent.com/41370460/153723627-96e6860e-1195-4e5e-b49f-f8a2a5c80ecb.mp4

prateekmedia commented 2 years ago

I have minimized all my upcoming comments as I misunderstood some things, but now I think this PR is great.

sanjidbillah commented 2 years ago

This is FadeTransition

https://user-images.githubusercontent.com/54928529/153738980-c7b4c62b-a563-4610-9099-d747622e0476.mp4

And this is ScaleTransition

https://user-images.githubusercontent.com/54928529/153739023-157fa4e8-20a1-40cc-b794-cc94e1546ef9.mp4

@prateekmedia Can you give me your example code with device information?

You should try like this. I used flutter default animation transition widget.

showPopover(
            context: context,
            transition: PopoverTransition.fadeTransition,
            transitionDuration: const Duration(seconds: 2),
            bodyBuilder: (context) => const ListItems(),
            onPop: () => print('Popover was popped!'),
            direction: PopoverDirection.bottom,
            width: 200,
            height: 400,
            arrowHeight: 15,
            arrowWidth: 30,
          );
prateekmedia commented 2 years ago

@sanjidbillah My platform is linux and the code I used to achieve Fade transition of the popover widget is here

To achieve I didn't send the scale values to Popover widget so it doesn't scale and then using FadeTransition as the parent of the popover.

prateekmedia commented 2 years ago

@sanjidbillah Also I used below code:

showPopover(
      context: context,
      barrierColor: Colors.transparent,
      shadow: [
        BoxShadow(
          color: context.borderColor,
          blurRadius: 6,
        ),
      ],
      bodyBuilder: (_) => bodyHere,
      width: 200,
      height: null,
      backgroundColor: Theme.of(context).cardColor,
    )
sanjidbillah commented 2 years ago

@prateekmedia I used same FadeTransition widget. Did you check with my code? i already checked it works. If you try with my code then give me the code i will check it.

You gave me your code not my example code.

Also i want know did you use real device or simulator give the device information.

prateekmedia commented 2 years ago

In short I wanted the popover to not scale but fade in when coming on screen.

sanjidbillah commented 2 years ago

@prateekmedia got it. If @minikin allow me to change ScaleTransition I will change it. I am waiting for code review response.

prateekmedia commented 2 years ago

I think he will if you are talking about fixing #48, regarding your current code If you feel it's useful then you can rename it to something like,

backgroundTransition: PopoverBackgroundTransition.fadeTransition,
sanjidbillah commented 2 years ago

@prateekmedia Right, But now i am thinking i will keep scale only for ScaleTransition.

prateekmedia commented 2 years ago

@prateekmedia Right, But now i am thinking i will keep scale only for ScaleTransition.

Do you mean for background or popover?

sanjidbillah commented 2 years ago

@prateekmedia For popover.

prateekmedia commented 2 years ago

@prateekmedia For popover.

Ok, but you should remove fix for #48 from description as this is not the fix for it. Thanks.

sanjidbillah commented 2 years ago

It will be fixed soon

sanjidbillah commented 2 years ago

@prateekmedia Please check i have fixed all transition.

prateekmedia commented 2 years ago

PR looks great but I have an issue. Issue: The transition for background and popover doesn't need to be same like I can want to have background transitioning with fade but the popover as Scale.

Probable solution: Add backgroundTransition property to popover and set its default value to fade, you may ask why fade, its because the package uses FadeTransition for background by default so this will be breaking change if its Scale for background.

TL;DR Separate transition for background as

backgroundTransition: PopoverTransition.fadeTransition,

sanjidbillah commented 2 years ago

@prateekmedia Brother i have made this change as you wish. Please run it again. I changed after watching your example video. background transitioning with fade but the popover as Scale. It was in the previous PR, Now you are telling me to go back to the previous PR again? I think we need to wait until @minikin code review. Otherwise it can become an unncessary problem.

minikin commented 2 years ago

@sanjidbillah @prateekmedia What I'd like to propose is to think about how we can implement generic abstraction about transition animation. And do not limit our users to PopoverTransition type. We can provide ScaleTransition as the default option.

prateekmedia commented 2 years ago

@minikin Something like a transitionBuilder property

sanjidbillah commented 2 years ago

@minikin @prateekmedia Please check new PR. I have added popoverBuilder now user can use any animation also i set ScaleTransition as default. I keep PopoverTransition Type. User can disable popover Transition by PopoverTransition.none by default it will use ScaleTransition. If you keep ScaleTransition for every animation i will remove it just let me know.

prateekmedia commented 2 years ago

@minikin Any update on this?

prateekmedia commented 2 years ago

@minikin Any reason why this PR is not merged?

prateekmedia commented 2 years ago

@minikin Can you review the changes and comment

sanjidbillah commented 2 years ago

I tried but i found a similar open issue here. @minikin any suggestion?

codecov[bot] commented 2 years ago

Codecov Report

Merging #50 (7633a93) into main (4299350) will decrease coverage by 0.59%. The diff coverage is 62.50%.

@@            Coverage Diff             @@
##             main      #50      +/-   ##
==========================================
- Coverage   90.98%   90.39%   -0.60%     
==========================================
  Files          10       10              
  Lines         455      458       +3     
==========================================
  Hits          414      414              
- Misses         41       44       +3     
Impacted Files Coverage Δ
lib/src/utils/popover_utils.dart 100.00% <ø> (ø)
lib/src/popover.dart 65.00% <50.00%> (-12.78%) :arrow_down:
lib/src/popover_context.dart 100.00% <100.00%> (ø)
lib/src/popover_item.dart 80.88% <100.00%> (+0.28%) :arrow_up:

:mega: We’re building smart automated test selection to slash your CI/CD build times. Learn more

minikin commented 2 years ago

@sanjidbillah We just need to update an example with a more applicable custom transition, and we'll be ready to merge.

sanjidbillah commented 2 years ago

@minikin I think I am not good at documentation but I will try -_-