Async promo dialog #6467
Async promo dialog #6467
Conversation
| import org.wordpress.android.ui.accounts.login.Login2FaFragment; | ||
| import org.wordpress.android.ui.accounts.login.LoginEmailFragment; | ||
| import org.wordpress.android.ui.accounts.login.LoginEmailPasswordFragment; | ||
| import org.wordpress.android.ui.accounts.login.LoginEpilogueFragment; |
| void inject(ReaderPostPagerActivity object); | ||
|
|
||
| void inject(EditorReleaseNotesActivity object); | ||
| void inject(ReleaseNotesActivity object); |
| resizePhotoPicker(); | ||
| } | ||
|
|
||
| // If we're showing the Async promo dialog, we need to redraw it so it takes into account |
There was a problem hiding this comment.
not sure what the comment meant, it should continue but ends abruptly?
There was a problem hiding this comment.
Woops! Fixed in 0a81c29.
| */ | ||
| public class PromoDialogAdvanced extends PromoDialog { | ||
| public static class Builder extends PromoDialog.Builder { | ||
| @StringRes int linkId; |
There was a problem hiding this comment.
can we make these linkResId, buttonNegativeResId, descriptionResId etc?
| protected static PromoDialogAdvanced newInstance(Builder builder) { | ||
| PromoDialogAdvanced fragment = new PromoDialogAdvanced(); | ||
| Bundle args = new Bundle(); | ||
| args.putInt("drawableId", builder.drawableId); |
There was a problem hiding this comment.
Can we move the "drawableId", "titleId", etc strings to private static final String...?
There was a problem hiding this comment.
An excellent idea - done in f4e7063.
|
This is looking great! I'm thinking the illustration should have slightly less top/bottom padding, but let's roll with this for now. |
| R.string.async_promo_description, | ||
| android.R.string.ok) | ||
| // TODO: Re-enable once a release notes page exists for Async | ||
| // .setLinkText(R.string.async_promo_link) |
There was a problem hiding this comment.
let's create an issue so we track this one before release
| }); | ||
| } else { | ||
| btn.setOnClickListener(mPositiveButtonOnClickListener); | ||
| } |
There was a problem hiding this comment.
We're lacking a dialog cancel() call here like we do in https://github.com/wordpress-mobile/WordPress-Android/pull/6467/files#diff-1999fe3664a86fade02c8922f6857c75R134
The default click handler does issue a cancel:
https://github.com/wordpress-mobile/WordPress-Android/pull/6467/files#diff-42c777438a1a186bb67bd763a279089eR100
I’m wondering, shouldn’t we make sure that when you set a handler we also cancel the dialog, either by giving the new handler that responsibility, or if not, we could maybe set our own anonymous handler and call the other listener within it, like this:
btn.setOnClickListener(new View.OnClickListener() {
@Override
public void onClick(View view) {
getDialog().cancel();
mPositiveButtonOnClickListener.onClick(view);
}
});
There was a problem hiding this comment.
I'm not sure we should hard-code that behavior. It's conceivable that you might not want to dismiss the dialog on the positive button, or do something else with the dialog first, so to me it makes sense to leave it up to the dialog 'client' to dictate the behavior.
|
|
||
| Button buttonPositive = (Button) view.findViewById(R.id.promo_dialog_button_positive); | ||
| buttonPositive.setText(mButtonPositiveId); | ||
| buttonPositive.setOnClickListener(mPositiveButtonOnClickListener); |
There was a problem hiding this comment.
None of this is necessary as the positive button's text and listener are already being set in the base class
There was a problem hiding this comment.
I agree it's not necessary as things currently stand. But, we don't have a way of checking that the layout we're using matches the layout of the parent class. So I left in the setup so it's explicit what view names each dialog expects to be there. This is around where the usefulness of the inheritance pattern wears thin 😞
There was a problem hiding this comment.
Let's leave it as is - I think the layouts will be kept similar for the context of use we are giving to these Dialogs, but really no harm in leaving this and re-inforcing the need to have a specific promo_dialog_button positive set in the view.
|
Nice job @aforcier @iamthomasbishop ! |
Adds a promo dialog, shown the first time a user presses the 'Publish' button:
Note: For now, the 'How does it work?' text is hidden. Closer (or just after) the beta we'll put up a post on mobile make, link to that, and unhide the link text.
I also added a new
PromoDialogAdvanced, intended for reuse for any dialogs requiring a link or negative button field (the existingPromoDialogEditornow extends this one, adding an extra title field for 'Beta'). The pre-existingPromoDialogcan be used for simple (image, title, description, positive button) promos.Also some design changes:
cc @iamthomasbishop (no visual changes to the async promo since we last spoke)