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

adding a custom dialog #74

Merged
merged 4 commits into from
May 8, 2024
Merged

adding a custom dialog #74

Changes from 3 commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
b95e8b8
adding a custom dialog
Apr 17, 2024
e9630ad
changing DialogFragment to dialog
Apr 23, 2024
ce3f481
Updating shaky flow for custom dialog
Apr 24, 2024
9c8fd24
moving custom dialog to ShakeDelegate
May 8, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
17 changes: 15 additions & 2 deletions shaky/src/main/java/com/linkedin/android/shaky/Shaky.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -17,6 +17,7 @@

import android.app.Activity;
import android.app.Application;
import android.app.DialogFragment;
import android.content.BroadcastReceiver;
import android.content.Context;
import android.content.Intent;
Expand Down Expand Up @@ -50,9 +51,10 @@ public class Shaky implements ShakeDetector.Listener {

private static final String SEND_FEEDBACK_TAG = "SendFeedback";
private static final String COLLECT_DATA_TAG = "CollectFeedbackData";
private static final String CUSTOM_DIALOG_TAG = "CustomDialog";

private static final long SHAKE_COOLDOWN_MS = TimeUnit.SECONDS.toMillis(5);

private DialogFragment customFragment;
private final ShakeDelegate delegate;
private final ShakeDetector shakeDetector;
@Nullable
Expand Down Expand Up @@ -186,12 +188,23 @@ public void hearShake() {
arguments.putInt(ShakySettingDialog.SHAKY_CURRENT_SENSITIVITY, delegate.getSensitivityLevel());
SendFeedbackDialog sendFeedbackDialog = new SendFeedbackDialog();
sendFeedbackDialog.setArguments(arguments);
sendFeedbackDialog.show(activity.getFragmentManager(), SEND_FEEDBACK_TAG);
if (customFragment != null) {
customFragment.show(activity.getFragmentManager(), CUSTOM_DIALOG_TAG);
Copy link

Choose a reason for hiding this comment

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

tiny nit - it seems like a lot of the logic above this line could probably go in the else block since we're not actually doing anything with the sendFeedbackDialog right? And then similar question for the shakyFlowCallback on 171 and on 197.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I kept the code as close to existing code as possible and i feel a more condensed if else loop would increase readability and understanding of the logic a lot easier. If it's not a logic breaking change and just for aesthetics, I really appreciate the suggestion but would like to keep the code this way so the PR is short too and easy to track down for other developers

} else {
sendFeedbackDialog.show(activity.getFragmentManager(), SEND_FEEDBACK_TAG);
}
if (shakyFlowCallback != null) {
shakyFlowCallback.onUserPromptShown();
}
}

/**
* Custom dialog to be shown before the feedback flow
*/
public void setCustomDialog(@NonNull DialogFragment fragment) {
customFragment = fragment;
Copy link

Choose a reason for hiding this comment

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

wouldn't it make more sense to add this field to the ShakeDelegate? It's giving me the vibe that communicating little customizations like this is the purpose of the delegate.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

So if you look at the current flow altogether, you can only alter the dialog title and message and not the button text or behavior. And also, the delegate starts the feedback flow whereas we are adding a feature that could be run before the feedback flow if a custom dialog is provided

}

/**
* @return true if a shake happened in the last {@link #SHAKE_COOLDOWN_MS}, false otherwise.
*/
Expand Down