search

duytq94 / flutter-intro-slider

Simple and configurable app introduction slider for Flutter
https://pub.dartlang.org/packages/intro_slider
MIT License
677 stars 141 forks source link

Intro Slider overlaps primary application MaterialApp #20

Closed freitzzz closed 5 years ago

freitzzz commented 5 years ago

Hi @duytq94 !

First of all thank you for creating this plugin and for bringing back onSkipPress on 1.2.1 :) Since IntroSlider builds itself using a MaterialApp, it overlaps the primary application MaterialApp, removing it's title and theme and other properties. Is there a way to prevent this?

Best regards, freitzzz

freitzzz commented 5 years ago

Added a non required key to intro_slider that should point to application root MaterialApp in order to maintain it's properties. If you appreciate the solution @ me so I can pull request :)

https://github.com/freitzzz/flutter-intro-slider/commit/b6aa0b942b303e4ddac303993c2e1f4da4f125ef

freitzzz commented 5 years ago

@duytq94

From my point of view, a plugin should never modify primary application properties, instead it should behave with how the application is built on. Given this, I removed plugin locale and status bar hide support, as these should be defined previously by the application developer(s)

(e.g. If I want my Intro slider to act differently for arabic users, then my application should recognize that the user is arabic, defining it's locale to arabic in application main, and not giving such responsibility to a plugin)

j3g commented 5 years ago

hello =) I really love this plugin. Niiiice. I also came across this issue. There should only be one MaterialApp() per application. The widget to use in its place is Scaffold. Using MaterialApp() again will also mess with app navigation, "The MaterialApp configures the top-level Navigator to search for routes in the following order..." This is the issue I'm having, with navigation.

duytq94 commented 5 years ago

Added a non required key to intro_slider that should point to application root MaterialApp in order to maintain it's properties. If you appreciate the solution @ me so I can pull request :)

freitzzz@b6aa0b9

Hi, @freitzzz that's a good way, please pull request and I'll checking, thank a lot for this report!

duytq94 commented 5 years ago

Hi, @j3g We will solve with @freitzzz 's solution. Or if you find a better way to do it, please tell me.

freitzzz commented 5 years ago

Hi, @j3g We will solve with @freitzzz 's solution. Or if you find a better way to do it, please tell me.

Hi @duytq94 I've changed my solution to removing the MaterialApp and replacing it by a Scaffold, giving the responsibility of the widgets localization to the developer of the application, not the plugin. Do you still want me to pull request?

Cheers, freitzzz

duytq94 commented 5 years ago

Hi @freitzzz I think we should keep localization, it's just an option and it doesn't cause any effect to the plugin. Give more option will help the user make more choice. Btw, this localization is made by a member's pull request. And they need it, so I think it's still helpful with them.

duytq94 commented 5 years ago

But if removing the MaterialApp and replacing it by a Scaffold, seem can't add localization anymore, right? Hope there is another way to solve both problems, if not, I think you're right, I have to remove localization and let it for the developer of the application.

duytq94 commented 5 years ago

Hi @freitzzz I removed the MaterialApp and localization too, please check the lastest version, thank you a lot.

freitzzz commented 5 years ago

Hi @freitzzz I removed the MaterialApp and localization too, please check the lastest version, thank you a lot.

Thank you for the update :)

j3g commented 5 years ago

Thank you for the update. I created a patch and I thought I submitted a pull request but I discovered that I did it in my own fork...so oops my bad, I was trying to contribute. Cheers!