-
Notifications
You must be signed in to change notification settings - Fork 1.4k
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
Refactoring dialogs #541
Comments
I would like to work on this issue @grzesiek2010 |
@UrjaPawar sure but it's rather for discussion now. We have to chose the best option for us and then we can start work with the code. Please review options mentioned above and let us know what you think. Maybe you can add your own proposal - don't hasitate if you know a better solution. |
@grzesiek2010 We should also provide an option to create custom dialogs. For that we would need to extend DialogFragments and then override the |
@grzesiek2010 Yes, let me consider each of them carefully. Since app is going to be updated according to new design as stated here, I think this https://github.com/michael-rapp/AndroidMaterialDialogone should be preferred |
@shobhitagarwal1612 I'm not sure it depends on te approach we choose. For example that solution provide a wide range of dialogs so maybe we can releace all our dialogs using one of ready-made solutions. |
@grzesiek2010 I agree that using an open source library is more handy than writing our own code for the implemented solution.But the library you proposed doesn't have many stars or forks. That means that library might not be fully tested and may contain potential bugs. I found another library on github which is very popular. |
@shobhitagarwal1612 I saw that solution as well and I tested a sample app but in that solution dialogs are not visible after activity recreation. It's a problem but maybe we can change that I don't know. @UrjaPawar you can test that solution as well. |
@grzesiek2010 Can you please describe how to recreate the issue that you encountered while using the sample app |
@shobhitagarwal1612 |
@grzesiek2010 I checked out the sample app, seems we need to override onConfigurationChanged method and find out more of these issues |
@UrjaPawar if we use dialogs (with DialogFragment) it should work out of the box. The solution attahced by me https://github.com/michael-rapp/AndroidMaterialDialog works well. |
@grzesiek2010 Yes! I saw that thing now |
I'm not convinced of the need to introduce an external dependency for this. There are some risks in doing so. For example, we change the I do think they have nice inheritance hierarchies for dialogs that we could consider borrowing ideas from. I tend not to like having a single creator because it groups together unrelated behavior. I think there is some value in hiding some of the implementation details for dialog types that are used over and over again but I believe it should be up to the clients of those abstractions to do things like specify the actions of the dialog buttons. This specific comment is in reaction to @nribeka @batkinson @yanokwa Would be interested in some additional opinions. |
I think we should take a step back and ask ourselves what are we trying to get with replacing the dialogs. Weigh in the pros and cons we're getting by replacing the dialogs. Are we just trying to replace the dialogs approach because different activities have their own dialogs or is actual problem caused by we're using too much dialogs inside the apps that can be replaced with different approach? I'm not a fan of introducing a lot of external dependencies in apps too. And if we decided to introduce dependency, we need to look at how mature the dependency are (how many forks, how many stars, how many open issues, how active is the development cycle). I worked with a lot of javascript lately and almost every day there's new library come out and also another library dead in the water 🤣 |
I didn't say that we have to use an external dependencies it's only an option we can introduce our own solution. |
@grzesiek2010 And the cons? |
@yanokwa At the moment I don't know any con. It's only time we have to spend :) |
Hi @grzesiek2010 i would like to give a piece of thought regarding this issue have a look at #610 |
@grzesiek2010 and @lognaturel inside the FileChooserList activity from Line 217 to Line 219
In the line 218 instead of setButton(Deprecated) it should be setPositiveButton. http://stackoverflow.com/questions/9723302/alert-dialog-inside-an-onclick-listener-method |
I want to take another step back! Our goal is to reduce redundancy across the code base. I'd rather not use an external dependency for this if we don't have to, but before we decide, we need to know what we have.
@grzesiek2010 Can you take the lead on this? |
@yanokwa sure |
Here is a list of our dialogs https://docs.google.com/document/d/1pZnr7wll-Wiw7lAakkgQlSEH4i683HZMdfjc8OUt-yc/edit?usp=sharing |
@grzesiek2010 That is super helpful. Good news is we really don't have that many different types. I think it would be best to address this after we've started using the Android Support Libraries in #912, right? @shobhitagarwal1612 |
@lognaturel yes we can wait for #912 |
@grzesiek2010 @shobhitagarwal1612 I think making the look and feel of dialogs consistent will be an important part of making the UI feel fresh and modern. I've marked it as part of the UI Refresh project but that doesn't necessarily mean @shobhitagarwal1612 has to be the one to do it. Now that #912 is merged, we can start thinking about what the best abstractions would be and when this should be done. Should it happen in |
@grzesiek2010 @shobhitagarwal1612 Dialogs are rather chaotic right now and I think making them consistent and matching material design guidelines seems it would be a good, non-controversial next UI improvement that would provide high value for the future by making it easier to add additional, consistent dialogs when needed. Looking at the dialog list, have you decided some of the abstractions that you'd like to introduce? Do you have a code structure to propose? Changes to propose across the board (for example, perhaps the "information" icon that a lot of dialogs have isn't needed anymore)? |
@lognaturel yes my proposal is ready #1226 |
For clarity, I want to make sure we're all on the same page regarding the goals here. I think the number 1 goal is to make sure dialogs have a consistent look and feel that matches modern guidelines. A secondary and also important goal is that the code for generating dialogs is consistent. This will help avoid errors moving forward and make it easier to add further dialogs as needed. Does everyone agree with those goals or want to make any changes to them? Keeping those in mind, @grzesiek2010 @shobhitagarwal1612, can the two of you please write up a description of the proposed approach, alternatives you considered, how you made the decisions you made, examples you looked at and other such information so that it's easier to provide feedback and make sure that we are going in the right direction. This change has far-reaching consequences and we want to make sure it is approached in a thoughtful way. A couple of specific questions I have:
@srsudar We were talking about this briefly recently. Do you happen to know of any good examples that use |
My order is a little bit different because I think the most important thing is to manage dialog easier and avoid some issues related to the current approach, layout is less important to me but generally I agree with you. I know this approach from my last project (TaroWorks) and it is still used as I know and works well. I wasn't able to find any other good solution without using external dependencies (I know you want to avoid it and me too). It's not necessary to keep @shobhitagarwal1612 what about you? |
I don't have any good pointers to follow. A static creator function as #1226 is common practice and makes sense to me, as long as it is testable which I think it would be. I've begun judiciously avoiding I looked at google-samples/android-architecture and android-architecture-components, but none of those projects appear to use dialogs. I'd love to rely less on dialogs, as I find them finicky and I don't like |
@grzesiek2010 have you considered BottomSheets as alternative to some dialogs? |
@Yurii-Laguta at the moment we use BottomSheets only for #1242 but I'll keep it in mind. |
As for DialogFragments I would suggest in order to avoid Interfaces hell to use event bus such as Otto and Builder pattern similar existing AlertDialog
I was surprised there is no Base activity. Because for example toolbar setup looks duplicated in all activities. |
Looks like the Otto README says it's deprecated in favor of rxjava and friends. The times I've used rxjava I've loved it, but I'm not familiar enough with it to have good intuitions about how to use it for dialogs. |
@srsudar that is interesting. After deeper reading of blog post about migrating from Otto to RxJava:
Given that this project doesn't have RxJava yet, and the 'steep learning curve' of it, I assume the main maintainers would prefer to go with Otto or EventBus. If it were my choice I would go with RxJava because nowadays it seems like mainstream. @lognaturel @yanokwa ^^^ someone have to make a decision here. |
I'm closing this issue, a lot has change since it was reported. |
There are lots of dialogs in our app. I think we really need to refactor this part of code as it tooks much code and it's really annoying.
First of all we should use DialogFragment of course (thanks to that we don't have to worry about activity recreation - our dialog is still visible and we don't need additional code to ensure that).
The second thing is as I mentioned we have lots of dialogs and I'm not sure creating separate class for each is a good idea. I proposed this solution https://github.com/grzesiek2010/collect/blob/c074d12ac6058db3760844fd55a1a558a658752a/collect_app/src/main/java/org/odk/collect/android/utilities/ODKDialogFragment.java it wasn't finished it was only a proposal (one creator class).
I found something intresting on github as well:
https://github.com/michael-rapp/AndroidMaterialDialog
Let's discuss!
The text was updated successfully, but these errors were encountered: