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

Dialog Screenshot #221

Merged
merged 4 commits into from
Mar 24, 2021
Merged

Dialog Screenshot #221

merged 4 commits into from
Mar 24, 2021

Conversation

dennisdeng2002
Copy link
Contributor

Add the ability to pass a Dialog to Maoni.start(), which will take a screenshot of the Dialog window. I use BottomSheetDialogFragment pretty extensively for one of my apps, and it'd be nice if a user could capture feedback for the Dialog window as opposed to just the underlying Activity

dennisdeng2002
dennisdeng2002 commented Mar 21, 2021
View reviewed changes
maoni-sample/src/main/java/org/rm3l/maoni/sample/ui/MaoniBottomSheetDialogFragment.kt
import org.rm3l.maoni.sample.R
import org.rm3l.maoni.sample.utils.MaoniUtils

class MaoniBottomSheetDialogFragment : BottomSheetDialogFragment() {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Added custom BottomSheetDialogFragment to test in sample app

maoni-sample/src/main/java/org/rm3l/maoni/sample/utils/MaoniUtils.kt
import org.rm3l.maoni.sample.extensions.getMaoniFeedbackBuilder
import org.rm3l.maoni.sample.extensions.getMaoniFeedbackHandler

object MaoniUtils {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Refactored setup code into a MaoniUtils class

maoni/src/main/java/org/rm3l/maoni/Maoni.java
* @param dialog the caller dialog
*/
public void start(@Nullable Dialog dialog) {
Activity activity = ContextUtils.scanForActivity(dialog.getContext());
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Added scanForActivity in case the Dialog context isn't an activity (i.e. ContextThemeWrapper)

maoni/src/main/java/org/rm3l/maoni/utils/ContextUtils.java
@@ -53,8 +55,17 @@ public static Object getBuildConfigValue(@NonNull final Context context,
.getField(fieldName)
.get(null);
} catch (final Exception e) {
//No worries
e.printStackTrace();
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Removed printStackTrace since it leads to some unnecessary noise for apps that integrate with Maoni

@dennisdeng2002
Copy link
Contributor Author

@rm3l Not sure why CI build is failing, any ideas?

* What went wrong:
Execution failed for task ':maoni-sample:processReleaseMetadata'.
> no JSON input found

@rm3l rm3l self-requested a review March 23, 2021 23:36
@rm3l
Copy link
Owner

rm3l commented Mar 23, 2021 •
edited
Loading

@dennisdeng2002 Thanks for this PR, which adds a nice and useful feature 👍🏿
I'll review it as soon as possible.

As per the CI, yesterday, I had few other PRs not passing, but still couldn't figure out why. Re-running the corresponding Workflows fixed the issue.
I just relaunched the Workflow for this PR. Let's see how it goes then.

@rm3l
Copy link
Owner

rm3l commented Mar 24, 2021 •
edited
Loading

Looks like the CI issue still persists.
I think GitHub Secrets are not available in forks. This is why one of the Gradle tasks that relies on a file generated off of those Secrets does not pass.
4b9f99d and 9a1ae57 should fix this issue.
Could you please rebase your branch onto master, so we can see if the CI is ok now ?
Thanks.

@dennisdeng2002
Copy link
Contributor Author

Looks like the CI issue still persists.
I think GitHub Secrets are not available in forks. This is why one of the Gradle tasks that relies on a file generated off of those Secrets does not pass.
4b9f99d and 9a1ae57 should fix this issue.
Could you please rebase your branch onto master, so we can see if the CI is ok now ?
Thanks.

Yeah was thinking it might be fork related, let me rebase and see

rm3l
rm3l approved these changes Mar 24, 2021
View reviewed changes
Copy link
Owner

@rm3l rm3l left a comment

Choose a reason for hiding this comment

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

@dennisdeng2002 Looks good to me! Thanks again for this PR. I'll merge it as soon as possible.

@rm3l rm3l merged commit 50fb3ca into rm3l:master Mar 24, 2021
@dennisdeng2002 dennisdeng2002 deleted the dialog-screenshot branch March 24, 2021 22:50
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@rm3l rm3l rm3l approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@dennisdeng2002 @rm3l