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.

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

Fix emoji only filter #393

Merged
merged 19 commits into from
Nov 6, 2019
Merged

Fix emoji only filter #393

merged 19 commits into from
Nov 6, 2019

Conversation

mario
Copy link
Collaborator

@mario mario commented Oct 3, 2019

Previously, when you enabled emoji-only filter and focused the edittext the emoji popup would appear.

In the future, when you tried focusing it for the second time only a keyboard would open as in reality edittext is already focused.

We now clear the focus after dismissing the emoji popup in case keyboard input is disabled.

mario and others added 8 commits October 2, 2019 12:40
Signed-off-by: Mario Danic <mario@lovelyhq.com>
Signed-off-by: Mario Danic <mario@lovelyhq.com>
Signed-off-by: Mario Danic <mario@lovelyhq.com>
Signed-off-by: Mario Danic <mario@lovelyhq.com>
Signed-off-by: Mario Danic <mario@lovelyhq.com>
Signed-off-by: Mario Danic <mario@lovelyhq.com>
Signed-off-by: Mario Danic <mario@lovelyhq.com>
Signed-off-by: Mario Danic <mario@lovelyhq.com>
Signed-off-by: Mario Danic <mario@lovelyhq.com>
Signed-off-by: Mario Danic <mario@lovelyhq.com>
Signed-off-by: Mario Danic <mario@lovelyhq.com>
@mario
Copy link
Collaborator Author

mario commented Oct 3, 2019

@vanniktech mind fixing the warnings here?

@vanniktech
Copy link
Owner

@mario I can do that once you're rebased

@mario
Copy link
Collaborator Author

mario commented Oct 14, 2019

@rubengees fancy fixing warnings? My head is a mess today.

@rubengees
Copy link
Collaborator

@mario Sure, done.

@rubengees
Copy link
Collaborator

I found a way to break out of the emoji picker and show a normal keyboard.

Steps:

  • Open keyboard.
  • Open input method dialog (for me on Android 10 in the bottom right).
  • Select other keyboard.

@rubengees
Copy link
Collaborator

PMD reports the EmojiPopup as a possible god class now due to it's size, that might be harder to fix...

@vanniktech
Copy link
Owner

Ignore it

vanniktech
vanniktech previously approved these changes Oct 16, 2019
@@ -23,6 +23,7 @@ dependencies {
testImplementation rootProject.ext.testing.assertJ
testImplementation rootProject.ext.testing.robolectric
testImplementation rootProject.ext.testing.privateConstructor
androidTestAnnotationProcessor deps.support.test.autoService
Copy link
Owner

Choose a reason for hiding this comment

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

What do we need this for?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

One of the lints complained about it missing.

@@ -100,7 +101,8 @@ ext {
junit : 'junit:junit:4.13-beta-3',
robolectric : 'org.robolectric:robolectric:4.3.1',
privateConstructor: 'com.pushtorefresh.java-private-constructor-checker:checker:1.2.0',
assertJ : 'org.assertj:assertj-core:2.9.1'
assertJ : 'org.assertj:assertj-core:2.9.1',
autoService : 'com.google.auto.service:auto-service:1.0-rc4'
Copy link
Owner

Choose a reason for hiding this comment

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

same

@@ -68,6 +68,7 @@ dependencies {
androidTestImplementation deps.support.test.runner
androidTestImplementation deps.support.test.rules
androidTestImplementation deps.fastLaneScreenGrab
androidTestAnnotationProcessor deps.support.test.autoService
Copy link
Owner

Choose a reason for hiding this comment

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

same

@mario
Copy link
Collaborator Author

mario commented Oct 16, 2019

I found a way to break out of the emoji picker and show a normal keyboard.

Steps:

  • Open keyboard.
  • Open input method dialog (for me on Android 10 in the bottom right).
  • Select other keyboard.

@vanniktech did you test this? Since I don't even see the input method dialog?

@vanniktech
Copy link
Owner

did you test this? Since I don't even see the input method dialog?

Not yet

@vanniktech
Copy link
Owner

I found one way:

  1. Check the checkbox
  2. Click on the input field
  3. Click on the dialog button
  4. Dismiss the dialog
  5. Click on the input field (Normal keyboard is shown instead of the emoji one)

@mario
Copy link
Collaborator Author

mario commented Oct 23, 2019

Should be ok now @vanniktech and @rubengees

Signed-off-by: Mario Danic <mario@lovelyhq.com>
Copy link
Owner

@vanniktech vanniktech left a comment

Choose a reason for hiding this comment

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

Tested and this works. Nice work! Let's wait until @rubengees also approves

@vanniktech
Copy link
Owner

@rubengees @mario how should we proceed with PR's so that they don't stay open that long?

I propose:

  • At least one review
  • Waiting for a second review for up to 3 days

@mario
Copy link
Collaborator Author

mario commented Oct 28, 2019

Well in general I'm highly in favour of at least two reviews, but I guess we need a policy on how long we wait. I feel like 3 days is too little? How does 7 days work for you two?

@vanniktech
Copy link
Owner

Sure. I'm fine with 7 days!

@vanniktech
Copy link
Owner

I'll merge this now as discussed with @mario.

@vanniktech vanniktech merged commit 489bd66 into master Nov 6, 2019
@vanniktech vanniktech deleted the fix-emoji-only-filter branch November 6, 2019 21:17
@rubengees
Copy link
Collaborator

Sorry for being away so long, I agree regarding the proposed time for waiting.

@vanniktech
Copy link
Owner

No need to be sorry. I hope everything is fine @ruben

@rubengees
Copy link
Collaborator

Yes sure, was just a bit lazy in the last time 😄

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants