Page MenuHomePhabricator

[web] Added more utility functions for typeahead.
ClosedPublic

Authored by przemek on Nov 24 2022, 7:18 AM.
Tags
None
Referenced Files
Unknown Object (File)
Sun, Nov 24, 9:12 PM
Unknown Object (File)
Sun, Nov 24, 8:44 PM
Unknown Object (File)
Sun, Nov 24, 7:54 PM
Unknown Object (File)
Sun, Nov 24, 7:51 PM
Unknown Object (File)
Sun, Nov 24, 5:41 PM
Unknown Object (File)
Fri, Nov 22, 7:59 AM
Unknown Object (File)
Fri, Nov 22, 7:59 AM
Unknown Object (File)
Fri, Nov 22, 7:59 AM

Details

Summary

Added utility functions for calculating overlay position and
another one for generating actions that will end up in overlay.

Test Plan

Functions visible, but not called anywhere yet.
Will test in final diff.

Diff Detail

Repository
rCOMM Comm
Lint
No Lint Coverage
Unit
No Test Coverage

Event Timeline

tomek requested changes to this revision.Nov 25 2022, 3:10 AM
tomek added inline comments.
web/chat/mention-utils.js
42–45 ↗(On Diff #18818)

This change should've been made in the diff that introduced this code

75–82 ↗(On Diff #18818)

This logic is hard to follow ans can be simplified (subtracting offsetLeft from offsetLeft can be omitted)

92 ↗(On Diff #18818)

We should either use shorthand array syntax or read only array. When returning value generated in a function, we can use mutable array.

94 ↗(On Diff #18818)

What if there are multiple users without username? We probably should filter them out, but definitely we shouldn't have duplicated keys.

User id sounds like a lot safer option to use as a key.

104 ↗(On Diff #18818)

You can simplify it: if newSuffixText.length === 0 then newSuffixText[0] === undefined so the first condition isn't required

111 ↗(On Diff #18818)

Why do we need this?

This revision now requires changes to proceed.Nov 25 2022, 3:10 AM
przemek marked 6 inline comments as done.

Responded to inlines and fixed code.

web/chat/mention-utils.js
42–45 ↗(On Diff #18818)

Right, moved those changes to: https://phab.comm.dev/D5674

75–82 ↗(On Diff #18818)

yeah, I felt that it was clumsy, but couldn't find a better way. That's neat, thanks

92 ↗(On Diff #18818)

Changing to readonly

94 ↗(On Diff #18818)

Right, fixed both. Filtering out anonymous users and using id as a key.

104 ↗(On Diff #18818)

Ah right, forgot JS handles it that way.

111 ↗(On Diff #18818)

You served as a rubber duck. I've spent like 15 minutes trying to write up explanation and came up with a nicer way to write it. Now space character is included in newSuffixText so we can simply subtract its length and then move 1 character to the right, so the caret is after that one space.

Had some problems with our git workflow and left that as an artifact. Sorry for that, fixed now.

tomek added inline comments.
web/chat/mention-utils.js
86 ↗(On Diff #18841)

Now you can use a shorthand

144 ↗(On Diff #18841)

Maybe we can modify the code to ask for boundingClientRect only once?

This revision is now accepted and ready to land.Nov 25 2022, 9:00 AM