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

[IMPROVEMENT] Use Flutter Bloc architecture #5

Closed
urmilshroff opened this issue Oct 5, 2019 · 15 comments
Closed

[IMPROVEMENT] Use Flutter Bloc architecture #5

urmilshroff opened this issue Oct 5, 2019 · 15 comments
Assignees
Labels
enhancement New feature or request hacktoberfest Participate in Hacktoberfest 2019 by fixing this issue! help wanted Extra attention is needed

Comments

@urmilshroff
Copy link
Member

Implement Bloc architecture as described in this article to improve performance and scalability.

@urmilshroff urmilshroff added enhancement New feature or request help wanted Extra attention is needed hacktoberfest Participate in Hacktoberfest 2019 by fixing this issue! labels Oct 5, 2019
@urmilshroff urmilshroff changed the title Use Flutter Bloc architecture [IMPROVEMENT] Use Flutter Bloc architecture Oct 10, 2019
@notDmDrl
Copy link

I'd like to work on this issue. Is it still available for working on? thx

@urmilshroff
Copy link
Member Author

I'd like to work on this issue. Is it still available for working on? thx

Yes, but we've decided to work on it later.

@notDmDrl
Copy link

Just to clarify, when I'll be able to work on this? @urmilshroff

@urmilshroff
Copy link
Member Author

@notDmDrl you are free to work on it if you want, but currently there's a lot of changing code that may break things. So we've decided to work on this issue later as switching to Bloc architecture would require rewriting a lot of functions.

@notDmDrl
Copy link

@urmilshroff thx for clarification. Unfortunatelly I have like a week of spare time to work on your project and i really want to do it so I probably will start working on it in a few days and will try to make that Bloc pretty easy to add your future code to.

@urmilshroff
Copy link
Member Author

@notDmDrl Oh, okay. That sounds great! Just keep in mind that you follow the CONTRIBUTING.md guidelines properly because of the encryption we've used for our API keys.

Alternatively, you can also work on some of the other open issues if you'd like. Thanks in advance!

@notDmDrl
Copy link

notDmDrl commented Nov 4, 2019

Hi @urmilshroff, want to give quick update. So i already have some coding towards Bloc done, i've done some "testing" by launching app (my location was easily found), but suddenly it just stopped working at all. All i get now is error (i never touched this IntroPage at all):
I/flutter ( 3894): ══╡ EXCEPTION CAUGHT BY WIDGETS LIBRARY ╞══════════════ I/flutter ( 3894): The following assertion was thrown building MyIntroPage(dirty, dependencies: [MediaQuery], state: I/flutter ( 3894): _MyIntroPageState#60424): I/flutter ( 3894): 'package:liquid_swipe/liquid_swipe.dart': Failed assertion: line 43 pos 16: 'onPageChangeCallback != I/flutter ( 3894): null': is not true.
I already tried to completely uninstall app, revert all changes to code i made, and even tried downloading project again from git, still not working with exact same error. It also happens on emulator (with both, vanilla and modified projects).

I don't know if i will be able to fix this error today, if not, then i'll continue later this week.

@urmilshroff
Copy link
Member Author

all changes to code i made, and even tried downloading project again from git, still not working with exact same error. It also happe

I'm facing that same problem, but in the Driver app. I copy pasted some of the code from Rider to Driver, and while all of it seems to be just fine, that same error is thrown. See this issue.

For me, Rider works just fine as of now. I'm guessing there's something wrong in a recent update of the LiquidSwipe package, so everytime you run flutter pub get on a fresh clone it downloads the new buggy version.

@urmilshroff
Copy link
Member Author

Due to many breaking changes and complexities in code, it isn't really worth switching to BLoC architecture in the forseeable future.

If anybody already has a working solution, then please ping here. Closing the issue for now.

@notDmDrl
Copy link

Hi, considering that you decided to not swith to BLoC, i'll probably just send PR with what i have already done (from what i've read at that article you mentioned thats basically just a get location function, but i would also add a few more to BLoC if you want).

Sorry for not reporting/sending a PR sooner, i had no free time last week.

@urmilshroff
Copy link
Member Author

@notDmDrl Sure, feel free to send the PR. It'll be useful for future reference.

That's alright if you were busy, we were too. But we need to release the app by December and switching to BLoC now may delay the process.

@notDmDrl
Copy link

notDmDrl commented Nov 14, 2019

@urmilshroff Good, i'll try to finish everything i wanted in your project and send PR before the end of the week.

@notDmDrl
Copy link

@urmilshroff Hi, sorry for that delay, my laptop's hard drive corrupted that weekend. Just managed to finally get some data from there, including my progress on this issue.

So should i just send PR as the project is right now? Or should i adapt it to changes you've made recently?

@urmilshroff
Copy link
Member Author

@notDmDrl you can just send the PR as is. We'll refer to your code and implement it accordingly in December/January

@notDmDrl
Copy link

@urmilshroff Ok, i'll send it in a few minutes.

Also i just want to mention it here that backup i was able to get isn't with latest additions i made but main points from that article are here.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request hacktoberfest Participate in Hacktoberfest 2019 by fixing this issue! help wanted Extra attention is needed
Projects
None yet
Development

No branches or pull requests

3 participants