search

Skyost / RateMyApp

This plugin allows to kindly ask users to rate your app if custom conditions are met (eg. install time, number of launches, etc...).
https://pub.dev/packages/rate_my_app
MIT License
273 stars 111 forks source link

Refractor transitions #105

Closed Sadeesha-Sath closed 3 years ago

Sadeesha-Sath commented 3 years ago

Declared a dedicated [DialogTransition] class for transitions as discussed in #102

@Skyost

Skyost commented 3 years ago

Very nice !

Sadeesha-Sath commented 3 years ago

Hey I just noticed that I made a small mistake in this PR. Nothing breaking of course.

But some asserts are in the body of the constructor of Dialog Transition Class. Here they won't get executed. We should move them to the initializer(after the ':'). And change 'transitionType' to 'transition'. Can you change this in a future commit? It seems too minor to submit another PR tbh.

Just learned that asserts cannot be executed in constructor body. ✌ Couldn't check it as dartpad doesn't support asserts yet. Sorry. I'm still learning.

@Skyost

Skyost commented 3 years ago

@Sadeesha-Sath Yeah sure. I was planning to test everything and to make a release during next week anyway :wink:

Skyost commented 3 years ago

Well, the asserts give me an error with the dart analyzer. I think they're invalid for some reason.

A fix would be to either change them or completely get rid of them. I prefer the second solution because the parameters would be silently ignored (and that's all, no need to throw an error or something).

Sadeesha-Sath commented 3 years ago

I wrote them so people won't be able to change startoffset or alignment without the transition they are supported. So the same goes for trying them with transition: TransitionType.none . So that users would not feel like the arguments are not working. Maybe you're right. It is better to silently ignore them. They have the property documentation after all.

But I don't see why they would fail though

Skyost commented 3 years ago

But I don't see why they would fail though

Same but my analyzer keeps giving me errors when used in constant constructors.

Sadeesha-Sath commented 3 years ago

Maybe it's because you are using const constructors. The constructor cannot be const because we change transitionType to TransitionType.custom in the initializer when the user only passes customTransitionBuilder. Since the transitionType defaults to none and the function checks for transitionType before customTransitionBuilder, we have to change it to custom before the test. Maybe you can remove the check from initializer and it move to both showDialog functions before the check for transitionType happens. Then we can define the constructor as const.