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.

Sign up for GitHub

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

Jump to bottom

Use appcompat's material-themed dialogs #896

Closed
wants to merge 1 commit into from
Closed

Use appcompat's material-themed dialogs #896

wants to merge 1 commit into from

Conversation

larsgrefer
Copy link
Contributor

No description provided.

@larsgrefer larsgrefer mentioned this pull request Oct 8, 2015
Merged
@ZacSweers
Copy link
Contributor

Before I review: why?
On Wed, Oct 7, 2015 at 10:43 AM Lars Grefer notifications@github.com
wrote:

You can view, comment on, or merge this pull request online at:

#896
Commit Summary

  • Use appcompat's material-themed dialogs

File Changes

Patch Links:

—
Reply to this email directly or view it on GitHub
#896.

@larsgrefer
Copy link
Contributor Author

Because:

larsgrefer
larsgrefer reviewed Oct 8, 2015
View reviewed changes
app/src/main/java/com/github/pockethub/ui/repo/RepositoryViewActivity.java
import android.text.TextUtils;
import android.view.Menu;
import android.view.MenuItem;
import android.widget.ProgressBar;

import com.afollestad.materialdialogs.MaterialDialog;
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is the only class where the com.afollestad.material-dialogs:core:0.8.0.1@aar dependency was used

@larsgrefer
Copy link
Contributor Author

And because of #818

@ZacSweers
Copy link
Contributor

Appcompat dialogs are not part of the design library, and just because it
updates periodically doesn't mean we should drop it. Removing a dependency
isn't implicitly a good thing if it's good. I've found it to be much nicer
than appcompat's dialogs in terms of both UI and functionality. In light of
that, I don't think we're going to switch anytime soon.
On Thu, Oct 8, 2015 at 3:08 PM Lars Grefer notifications@github.com wrote:

And because of #818 #818

—
Reply to this email directly or view it on GitHub
#896 (comment).

@larsgrefer
Copy link
Contributor Author

In fact we are using the extra library for exactly one dialog (in RepositoryViewActivity) and i think for only one dialog we don't need an extra dependency

most of the changes i made are just moving from android.app.AlertDialog to android.support.v7.app.AlertDialog

@ZacSweers
Copy link
Contributor

We should be moving all the dialogs to material dialogs then. There are
features of it that don't exist in the appcompat's dialogs as well, such as
sensible widths on tablets and more customizability. I don't know who did
the original addition of it here, but I very much prefer them to
appcompat's.
On Thu, Oct 8, 2015 at 11:20 PM Lars Grefer notifications@github.com
wrote:

In fact we are using the extra library for exactly one dialog (in
RepositoryViewActivity) and i think for only one dialog we don't need an
extra dependency

most of the changes i made are just moving from android.app.AlertDialog
to android.support.v7.app.AlertDialog

—
Reply to this email directly or view it on GitHub
#896 (comment).

@larsgrefer larsgrefer force-pushed the material/dialogs branch 2 times, most recently from cb24495 to f7f93b6 Compare October 9, 2015 18:07
@larsgrefer
Use AppCompat's AlertDialog instead of the platform one in order to a…
b1c11f6 
…chieve an platform independent material design
@larsgrefer
Copy link
Contributor Author

I've updated the pull request. Now it wont touch the material dialogs by afollestad.

@Meisolsson
Copy link
Contributor

This should be closed and we should instead just start moving from LightAlertDialog to the material dialogs lib

@larsgrefer
Copy link
Contributor Author

@Meisolsson Why not using this in the meantime until we move to affollestad's lib?

AppCompats Dialogs are material, too (in contrast to the current LightAlertDialog implementation)

@Meisolsson
Copy link
Contributor

Well this feels like a redundant step

@Meisolsson Meisolsson closed this Oct 10, 2015
@larsgrefer
Copy link
Contributor Author

Since i've already done the work, the only redundancy you're preventing, is clicking the merge pull request button twice

And in my opinion it's at least an improvement that alle the dialogs look material on all devices

@ZacSweers
Copy link
Contributor

Lars, with all due respect, you're ignoring a major rule in our CONTRIBUTING.md:

Please make sure you discuss these with us in the issue tracker before opening a pull request. It's good to get a conversation going first to make sure that everyone is on the same page, and this way you don't accidentally invest a lot of time into something we don't want to merge

Just because you've done work doesn't mean that you're entitled to having it merged. Had you discussed this with us first, we could avoided all this. Please respect our decisions.

@larsgrefer larsgrefer deleted the material/dialogs branch October 10, 2015 17:01
@StefMa StefMa mentioned this pull request Oct 11, 2015
Closed
@larsgrefer larsgrefer restored the material/dialogs branch October 11, 2015 15:22
@veyndan veyndan mentioned this pull request Jul 1, 2019
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@larsgrefer @ZacSweers @Meisolsson