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

Mentions #10953

Closed
wants to merge 2 commits into from
Closed

Mentions #10953

wants to merge 2 commits into from

Conversation

yenda
Copy link
Contributor

@yenda yenda commented Jul 14, 2020

working well on Android but not iOS?

@yenda yenda requested review from antdanchenko, churik and a team as code owners July 14, 2020 10:40
@yenda yenda force-pushed the mentions branch 2 times, most recently from fa769b3 to 1201a20 Compare July 14, 2020 10:42
@status-im-auto
Copy link
Member

status-im-auto commented Jul 14, 2020

Jenkins Builds

Click to see older builds (7)
Commit #️⃣ Finished (UTC) Duration Platform Result
✔️ 1201a20 #3 2020-07-14 10:53:34 ~10 min android 📦apk 📲
✔️ 1201a20 #3 2020-07-14 10:54:18 ~11 min android-e2e 📦apk 📲
✔️ 1201a20 #3 2020-07-14 10:55:56 ~12 min ios 📦ipa 📲
1201a20 #4 2020-08-04 15:02:19 ~24 sec ios 📄log
✔️ f944431 #4 2020-08-05 08:06:17 ~10 min android-e2e 📦apk 📲
✔️ f944431 #4 2020-08-05 08:06:21 ~10 min android 📦apk 📲
✔️ f944431 #5 2020-08-05 08:09:37 ~14 min ios 📦ipa 📲
Commit #️⃣ Finished (UTC) Duration Platform Result
✔️ eb14902 #5 2020-08-06 08:21:04 ~10 min android-e2e 📦apk 📲
✔️ eb14902 #5 2020-08-06 08:21:06 ~10 min android 📦apk 📲
✔️ eb14902 #6 2020-08-06 08:25:05 ~14 min ios 📦ipa 📲
✔️ 94a8665 #6 2020-08-06 10:40:26 ~10 min android-e2e 📦apk 📲
✔️ 94a8665 #6 2020-08-06 10:40:26 ~10 min android 📦apk 📲
✔️ 94a8665 #7 2020-08-06 10:44:09 ~14 min ios 📦ipa 📲

@hesterbruikman
Copy link
Contributor

hesterbruikman commented Jul 14, 2020

@yenda what do you suspect is not working?

Tested

Samsung S10e Android 10
iPhone 8 iOS 13
Fairphone 2 Android 7

  • Call mention list
  • Select mention
  • View mention
  • Tap on mention

Findings

  • No difference between Android 7 and 8

Issue 1

On iOS, need to tap again on the text input field to make the whole name a mention, not great, but happens all the time for me with Discord, not a blocker IMO. Confirm without the additional tap to get the name right gives the result in image below [:current "@Worthy Shallow Porcupin"]e It seems like the cursor does not move to the end of the name, have to manually tap to move it

Issue 2

On iOS, need to enter the first letter to pull up list to select name. Not a blocker IMO

@yenda @errorists can you please join #xyz to try?

Mentions on iOS

Frame 2

@hesterbruikman
Copy link
Contributor

In short, great work @yenda!

Let's dig some more for issues that really require addressing for a first mentions release. Most prominent for me is seeing if there is a way to send a notification on mention. I didn't receive any on Android.

@errorists
Copy link
Contributor

just to confirm I tested on iOS and the noticeable bugs that impacted the previous PR are still present here too, most notably deleting letters in input and inserting them right after.

@errorists
Copy link
Contributor

some notes,
• tapping on a mentioned username should open the profile of the person rather than starting a new chat
• usernames block the long press gesture of a message bubble
• we should find a way to insert a @ character into the keyboard, at least on iOS where that's very common to have one next to space
• the list could use an animation on change of states
• the list is using incorrect styling, bear in mind those aren't the regular list items components but a smaller custom one to fit more usernames into the list.
• the autosuggestion list is missing its drop shadow / elevation

@rasom
Copy link
Contributor

rasom commented Aug 5, 2020

Just rebased PR onto current develop

@rasom
Copy link
Contributor

rasom commented Aug 6, 2020

@hesterbruikman
Do we want to fix issue1/2 in this PR?

@errorists

just to confirm I tested on iOS and the noticeable bugs that impacted the previous PR are still present here too, most notably deleting letters in input and inserting them right after.

Do we have steps to reproduce and more details on this somewhere?

@errorists
Copy link
Contributor

@rasom I'd like to first confirm if that's still the case, @Ferossgp you told me earlier this was fixed by Eric doing 'something' to the way input is stored, correct?

@hesterbruikman
Copy link
Contributor

@hesterbruikman
Do we want to fix issue1/2 in this PR?
IMO we can fix these later. Better to focus efforts on notifications on iOS to get a notification of a mention

The issue @errorists described does need insurance that it's resolved as it caused a regression to message input in general. I'll check as you rebased onto develop. @errorists can you please do the same and check again on this rebased PR. my eyes are not a reliable source to capture that jitter issue

@hesterbruikman
Copy link
Contributor

hesterbruikman commented Aug 7, 2020

Some more findings and recapping earlier comments. I think all, except 1, should be addressed in a separate PR (bounty) if they are not regressions to text input in general. Functionality of receiving a notification when mentioned is more important

To address in this PR

To address in separate PR

  • (image 1) on iOS jumping back to start of mention when completed. Expectation: cursor remains at end of mention name
  • (image 2) on Android when entering first letter (e.g @f) to call mentions list, and successively selecting name from list, f is not replaced by name. Expectation: any character immediately following @ to be replaced by mention name
  • (image 2) on Android own random name is shown in list while ENS name is connected to chat key.
  • on Android when someone else mentions user A, and user A has ENS name, user A sees own name as e.g. hester.stateofus.eth, while other user see the name as e.g. @hester

Earlier comments

  • (original issue 1) (image 3) On iOS and Android, need to tap again on the text input field on list to make name a mention. Confirm without the additional tap to get the name right gives [:current "@hester"] Expectation: Manually enter name from list is parsed as mention name
  • (original issue 2) On iOS Android, need to enter the first letter to pull up list to select name. Expecation: list appears on @ > STILL RELEVANT
  • on Android and iOS tapping on a mentioned username should open the profile of the person rather than starting a new chat > STILL RELEVANT
  • usernames block the long press gesture of a message bubble > WORKS NOW ON IOS AND ANDROID
  • we should find a way to insert a @ character into the keyboard, at least on iOS where that's very common to have one next to space > STILL RELEVANT FOR IOS (Not sure it's possible on Android, can't find apps doing this)
  • the list could use an animation on change of states > STILL RELEVANT
  • the list is using incorrect styling, bear in mind those aren't the regular list items components but a smaller custom one to fit more usernames into the list. > STILL RELEVANT
  • the autosuggestion list is missing its drop shadow / elevation > STILL RELEVANT

cc @churik @Serhy
tested on Android 7, Android 10, iOS 13
context: 1:1, group chat, public chat, with and without ENS name

Frame 1

@errorists
Copy link
Contributor

I can confirm that the issue of jittery text input is gone on iOS 🎉
however something other happened in the meantime that I believe was ok before, the suggestion list now spans the entire screen
IMG_1610

should not be the case, it can only reach the bottom of the top bar, not cover it.
The animation on the entry looks wrong, it should be a translateY, currently it does a scale on the X axis.
I guess the issues have to do with the new input design, @Ferossgp ?
just to make sure it doesn't get lost, to add to Hester's earlier list: tapping on a mentioned username should open the profile and not start a new chat

@rasom
Copy link
Contributor

rasom commented Aug 10, 2020

Notification on Android shows chat key instead of ENS name or random name

@hesterbruikman this PR was only about suggestion in input, I can try to check what's wrong there but it looks totally unrelated to the rest of PR

@rasom
Copy link
Contributor

rasom commented Aug 11, 2020

@hesterbruikman
tiny update:

There are many more little bugs besides those which are already mentioned in the PR, and it is really hard to debug and fix them at the moment. So I'm going to try to implement parts of it separately from the scratch, because fixing existing implementation takes too long.

One comment regarding issues mentioned in the comments: you want to have mention to be parsed from the input without tapping on the user in suggestion, but that's not how it works in some other messengers (for instance in discord you actually have to tap on suggestions, otherwise mention will not be highlighted). So my question is: do we have a written spec on how it should work (not only tapping on suggestion) or do we use some other app as a reference?

@rasom
Copy link
Contributor

rasom commented Aug 11, 2020

on Android and iOS tapping on a mentioned username should open the profile of the person rather than starting a new chat

btw in Telegram/Discord the chat is opened in this case, not profile. So it would be really great to see the list of all those requirements somewhere.

@rasom
Copy link
Contributor

rasom commented Aug 11, 2020

Just to summarise it:

  1. we want to have "@" in keyboard, like discord have (but not whatsap, telegram)
  2. we want to parse mentions without tapping on them, like telegram does (but not discord, whatsap)
  3. we also want to open profile on tapping mention in the message, like whatsap does (but not telegram, discord)
  4. we want to highlight mention in the text input, like whatsap does (but not telegram, discord)

and so on, we need the full list

@errorists
Copy link
Contributor

@rasom I'm afraid there is no requirements list, we're making this up as we're going along, like say with the issue of tapping on mentioned username, I wouldn't have an opinion on it without trying it out in the live implementation first.

we want to have "@" in keyboard, like discord have (but not whatsap, telegram)

yes, on iOS where it's a possibility. I think it's a no brainer, on less tap to start a mention. Specifically it should be the emailAddress type. There's twitter which also has a #, it's the one Discord uses but it doesn't have a return.

we want to parse mentions without tapping on them, like telegram does (but not discord, whatsap)

yes, it's the better UX, if you write down the exact username following @ I think the expectation is clear that it should register as a mention

we also want to open profile on tapping mention in the message

yes, 100% defo

we want to highlight mention in the text input

yes, to confirm that this will register as a mention and not plain text

@rasom
Copy link
Contributor

rasom commented Aug 11, 2020

so what's happening next:

  1. I will create a tiny separate PR for opening profile on taping mention in the message
  2. I will create a separate PR which parses input on sending and transforms all metions into format readable by other clients (which means replacing mentions with a public key). At this stage the user will be able to mention someone via ens/name, but they won't see suggestions/highlighted mentions in the input.
  3. separate PR for suggestions
  4. separate PR for highlighted mentions in the input field
  5. Another tiny PR for "@" in keyboard on iOS

@rasom rasom closed this Aug 11, 2020
@rasom rasom deleted the mentions branch February 5, 2021 15:31
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
No open projects
Archived in project
Development

Successfully merging this pull request may close these issues.

5 participants