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

Additional builder parameters #40

Merged
merged 2 commits into from Jan 11, 2022
Merged

Conversation

maeddin
Copy link
Contributor

@maeddin maeddin commented Jan 8, 2022

Hello :)
I had the problem with this package that very many widgets were built, although you can not see them anyway. In addition, the overlay had performance problems, since this was above all widgets in the stack and below and not only above the top widget. This led to very poor performance for me with BackdropFilters.
So I modified the package to pass in the builder where in the stack the current widget is and also the data that is passed to the OverlayBuilder. So you have on the one hand the possibility to adjust also lower widgets in the stack depending on the swipe progress, and on the other hand to implement the topmost overlay more efficiently in some situations. In addition, you can now optionally not build the lower widgets more easily, since it is directly passed where they are located in the stack.
The overlayBuilder remains nevertheless further.
The disadvantage of this request are the three parameters that are now additionally passed to the builder.

Apart from this small problem I had, this package is really very well done and I want to commend this work here.

@HeavenOSK
Copy link
Owner

HeavenOSK commented Jan 9, 2022

@MaddinMade
Thanks for committing! I understand the problem from your commit.

this package that very many widgets were built

Isn't it just one additional widget? The widget which has stack number '-1' is for preparing to rewind. (If no preparing, the rewinding cannot provide production quality.)

This led to very poor performance for me with BackdropFilters.

I've not used BackdropFilters with SwipableStack. Could you explain an example please? I'd like to know your usecase.

I guess we could fix the following code which uses Opacity.

opacity: _isRewindTarget ? 0 : 1,

@maeddin
Copy link
Contributor Author

maeddin commented Jan 9, 2022

@HeavenOSK Thanks for the quick reply :)

Isn't it just one additional widget? The widget which has stack number '-1' is for preparing to rewind. (If no preparing, the rewinding cannot provide production quality.)

You're right, the additional widgets are no valid reasons for this pull request. I had admittedly misunderstood how this package worked as you can see in the following code:

// for efficiency reasons
if(stackIndex > 1) return SizedBox();

I've not used BackdropFilters with SwipableStack. Could you explain an example please? I'd like to know your usecase.

For example, if you want to apply a blur effect, you can use BackdropFilters. These will then also be applied to all widgets rendered below. However, there is also the alternative of using the ImageFiltered widget, which applies ImageFilters like Blur to only one widget, making it more efficient in many cases. For example, I applied ImageFiltered to the top swipe card using this pull request. This way I didn't have to apply BackdropFilters in the overlay and I was able to solve a low frame count issue on my end.

In general, however, my goal with this pull request was not to solve a specific problem. I rather wanted to make the package more flexible. Another use case for this is that you could change the background color of a card in the stack depending on the swipe progress. Currently, for such cases, you would have to build the whole card in the overlay and then make the changes on it.
Also, you can change the bottom cards depending on the swipe progress to apply individual effects to them (which I didn't need). Currently you can just modify the viewFraction for manipulating the second child.

I hope I didn't babble too much and you can unterstand my goal with this pull request.

I guess we could fix the following code which uses Opacity.

Using opacity with 0 and 1 is not inefficient, as far as I know. It only becomes so once you use other numbers with it.

@HeavenOSK HeavenOSK self-requested a review January 10, 2022 09:57
Copy link
Owner

@HeavenOSK HeavenOSK left a comment

Choose a reason for hiding this comment

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

@MaddinMade
Thanks for committing & conversation!

Your change makes sense to me. But I found a few points that could be made better, so it would be great if you could fix them.

Thanks.

lib/src/callback/callbacks.dart Outdated Show resolved Hide resolved
lib/src/enum/swipe_direction.dart Outdated Show resolved Hide resolved
@maeddin
Copy link
Contributor Author

maeddin commented Jan 11, 2022

@HeavenOSK
Your suggestions are very reasonable and understandable. I have implemented the changes. You should double check that I am still passing the correct values to the builders. I just tested it a bit with the help of the example app.

Copy link
Owner

@HeavenOSK HeavenOSK left a comment

Choose a reason for hiding this comment

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

LGTM!! 👍

Thank you for your contribution! This change looks perfect to me & it's wonderful beyond my imagination.

You should double check that I am still passing the correct values to the builders.

I checked your point and it seems to be OK! Thank you so much 🙏

Comment on lines +4 to +23
abstract class SwipeProperties {
const SwipeProperties({
required this.index,
required this.constraints,
required this.direction,
required this.swipeProgress,
});

///Index of the current item.
final int index;

///[Constraints] of the whole stack.
final BoxConstraints constraints;

///Direction of the current swipe action.
final SwipeDirection? direction;

///Progress of the current swipe action.
final double swipeProgress;
}
Copy link
Owner

Choose a reason for hiding this comment

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

Nice abstraction! 💯

@HeavenOSK
Copy link
Owner

HeavenOSK commented Jan 11, 2022

I'll release changes in this PR as v1.1.0 💡

@HeavenOSK HeavenOSK merged commit e21f969 into HeavenOSK:develop Jan 11, 2022
@HeavenOSK
Copy link
Owner

@MaddinMade
I released this change as v1.1.0! Thanks for your contribution! 🙏
https://pub.dev/packages/swipable_stack/changelog#110

@maeddin
Copy link
Contributor Author

maeddin commented Jan 11, 2022

@HeavenOSK
Thanks for referencing in the CHANGELOG

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.

None yet

2 participants