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

mentions #10693

Closed
wants to merge 1 commit into from
Closed

mentions #10693

wants to merge 1 commit into from

Conversation

yenda
Copy link
Contributor

@yenda yenda commented May 19, 2020 •
edited
Loading

fix #10509

current implementation looks good enough performance wise, can be further improved by using more local state as well as react on-key-press in most cases, only changing the "completion?" value instead of recomputing everything

@yenda yenda requested a review from a team as a code owner May 19, 2020 23:31
@ghost
Copy link

ghost commented May 19, 2020

Pull Request Checklist

  • Docs: Updated the documentation, if affected
  • Docs: Added or updated inline comments explaining intention of the code
  • Tests: Ensured that all new UI elements have been assigned accessibility IDs
  • Tests: Signaled need for E2E tests with label, if applicable
  • Tests: Briefly described what was tested and what platforms were used
  • UI: In case of UI changes, ensured that UI matches Figma
  • UI: In case of UI changes, requested review from a Core UI designer
  • UI: In case of UI changes, included screenshots of implementation

@status-im-auto
Copy link
Member

status-im-auto commented May 19, 2020 •
edited
Loading

Jenkins Builds

Click to see older builds (9)
❔ Commit #️⃣ Finished (UTC) Duration Platform Result
❌ bbb2b1a #1 2020-05-19 23:43:21 ~11 min android-e2e 📄log
❌ bbb2b1a #1 2020-05-19 23:43:31 ~11 min android 📄log
❌ bbb2b1a #1 2020-05-19 23:48:28 ~16 min ios 📄log
❌ 8eacf3b #2 2020-05-20 14:11:20 ~11 min android 📄log
❌ 8eacf3b #2 2020-05-20 14:11:20 ~11 min android-e2e 📄log
❌ 8eacf3b #2 2020-05-20 14:14:18 ~14 min ios 📄log
✔️ 1276574 #3 2020-05-20 15:52:01 ~11 min android-e2e 📦apk 📲
✔️ 1276574 #3 2020-05-20 15:52:09 ~11 min android 📦apk 📲
✔️ 1276574 #3 2020-05-20 15:54:53 ~14 min ios 📦ipa 📲
❔ Commit #️⃣ Finished (UTC) Duration Platform Result
❌ e75171e #4 2020-05-20 15:58:05 ~2 min ios 📄log
❌ e75171e #4 2020-05-20 15:58:09 ~2 min android 📄log
❌ e75171e #4 2020-05-20 15:58:10 ~2 min android-e2e 📄log
✔️ 26f395a #5 2020-05-22 07:21:30 ~11 min android-e2e 📦apk 📲
✔️ 26f395a #5 2020-05-22 07:21:33 ~11 min android 📦apk 📲
✔️ 26f395a #5 2020-05-22 07:28:26 ~18 min ios 📦ipa 📲

@yenda yenda requested review from cammellos and flexsurfer May 20, 2020 15:42
@yenda yenda self-assigned this May 20, 2020
@errorists
Copy link
Contributor

errorists commented May 21, 2020 •
edited
Loading

issues

  • input is broken, inserting a character deletes it first and places it again, this happens in a split second but is visible and super annoying. Concerns all text, not only mentions
  • Input has a different height when there's no text inside (looks like it's lower by 1px) and we only show the placeholder than when there's text inside. this is accompanied by blinking of the screen when text is first inserted.
  • text inside input is offset a bit to the bottom, it doesn't look like it's aligned properly
  • when I choose someone from the autocomplete list, the autocomplete persists, I need to choose the username again to dismiss it, placing the username again in the input
  • selecting someone from the autocomplete dropdown places the cursor after the @ character and not at the end of username
  • placing cursor inside a username, removes the mention text colouring for all text after the cursor
  • autocomplete doesn't always work for me, example: I type @b and I still see the complete list of usernames beginning with all letters, not only b
  • the autocomplete list is blinking annoyingly whenever it's summoned / dismissed / changes height. It should animate, please look at Discord it's also RN.
  • messages with a mention are blocking me from long pressing the message: I need to long press it on the exterior parts of message bubble without text to fire up the event
  • mentioning a username that's not in the chat (@here) leads to weird message formatting

IMG_1498 2

  • replying to a message with a mention, in the reply menu the entire chat key is visible instead of the username

IMG_1501

things missing

  • no @ character in the system keyboard
  • tapping a username inside a mentioned message should display that user profile

tested on iOS,
@churik please test this one diligently, I'm sure there are many more issues hidden here 🙏

rasom
rasom reviewed Jun 12, 2020
View reviewed changes
src/status_im/chat/models/input.cljs
This is used to quickly render the input field with the proper styling"
[text users mention-regexp]
(if mention-regexp
(loop [[text mention & rest] (string/split text mention-regexp)
Copy link
Contributor

Choose a reason for hiding this comment

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

Why not just reduce?

rasom
rasom reviewed Jun 12, 2020
View reviewed changes
src/status_im/chat/models/input.cljs
(def end-mention-regexp
#"(@(?:(?:[A-Z][a-z]*(?:\s[A-Z]?[a-z]*)?(?:\s[A-Z]?[a-z]*)?)|(?:[a-zA-Z0-9\.]*))$)")

(defn compute-all-text-parts
Copy link
Contributor

Choose a reason for hiding this comment

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

we need some tests for this function, it is huge and hard to reading

rasom
rasom reviewed Jun 12, 2020
View reviewed changes
src/status_im/chat/models/input.cljs
;; prepare regexp for currently mentioned users
mention-regexp
(when (seq mentioned)
(re-pattern (str "(@" (string/replace
Copy link
Contributor

Choose a reason for hiding this comment

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

Can we extract magic values to vars?

@flexsurfer flexsurfer closed this Jun 26, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@rasom rasom rasom left review comments

@cammellos cammellos Awaiting requested review from cammellos

@flexsurfer flexsurfer Awaiting requested review from flexsurfer

Assignees

@yenda yenda

Labels
None yet
Projects
No open projects
Legacy QA Automation Project
Archived in project
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Mentions
5 participants
@yenda @status-im-auto @errorists @rasom @flexsurfer