Page MenuHomePhabricator
Differential D6052

[web] Keyboard support for typeahead [12/13] - Scrolling thourgh overlay as user interacts with it
ClosedPublic
Actions

Authored by • przemek on Dec 27 2022, 5:21 AM.
Tags
None
Referenced Files
F3379997: D6052.id20166.diff
Wed, Nov 27, 8:22 PM
F3379989: D6052.id20179.diff
Wed, Nov 27, 8:20 PM
F3379972: D6052.id20609.diff
Wed, Nov 27, 8:13 PM
F3379931: D6052.id20599.diff
Wed, Nov 27, 7:58 PM
Unknown Object (File)
Sun, Nov 24, 4:07 AM
Unknown Object (File)
Sun, Nov 24, 12:23 AM
Unknown Object (File)
Sat, Nov 16, 12:02 AM
Unknown Object (File)
Wed, Nov 13, 5:26 AM
View All Files
Subscribers
ashoat
atul
ginsu
inka
kamil
tomek

Details

Reviewers
inka
kamil
ginsu
tomek
Commits
rCOMMfb601172deb0: [web] Keyboard support for typeahead [12/13] - Scrolling thourgh overlay as…
Summary

Overlay scrolls automatically when users moves through users suggested to mention.

Test Plan

Tested on video below:

Diff Detail

Repository
rCOMM Comm
Lint
Lint Not Applicable
Unit
Tests Not Applicable

Event Timeline

• przemek created this revision.Dec 27 2022, 5:21 AM
Herald added subscribers: atul, ashoat. · View Herald TranscriptDec 27 2022, 5:21 AM
• przemek added a parent revision: D6051: [web] Keyboard support for typeahead [11/13] - Use callbacks to interact with overlay.Dec 27 2022, 5:32 AM
• przemek added a child revision: D6053: [web] Keyboard support for typeahead [13/13] - Freeze users list when typeahead is visible..
Harbormaster completed remote builds in B14787: Diff 20166.Dec 27 2022, 5:32 AM
• przemek requested review of this revision.Dec 27 2022, 5:32 AM
• przemek edited the test plan for this revision. (Show Details)Dec 27 2022, 5:57 AM
• przemek attached a referenced file: F306054: diff-12.mov. (Show Details)
• przemek updated this revision to Diff 20179.Dec 27 2022, 6:36 AM

rebase

Harbormaster completed remote builds in B14796: Diff 20179.Dec 27 2022, 6:50 AM
• przemek added a reviewer: tomek.Jan 3 2023, 8:22 AM
tomek requested changes to this revision.Jan 4 2023, 6:30 AM
tomek added inline comments.
web/chat/typeahead-tooltip.react.js
149 ↗(On Diff #20179)

Maybe we should use scrollTo method?

web/utils/typeahead-utils.js
164–178 ↗(On Diff #20179)

This is a little complicated - could you add some tests?

This revision now requires changes to proceed.Jan 4 2023, 6:30 AM
• przemek updated this revision to Diff 20599.Jan 5 2023, 1:59 AM

rebased

Harbormaster completed remote builds in B15104: Diff 20599.Jan 5 2023, 2:13 AM
• przemek updated this revision to Diff 20609.Jan 5 2023, 3:48 AM
• przemek marked an inline comment as done.

Used scrollTo.
Added tests and run them.
There were some problems that I solved locally using fixes from here: https://linear.app/comm/issue/ENG-2654/problems-with-jest
I haven't pushed those fixes anywhere as it is to be discussed yet, but I found it acceptable to
push tests even if they fail currently (due to lack of mocks as mentioned on linear).
Similar thing is happening to tooltip-utils.test.js tests and it was ignored in the past I guess.

Harbormaster completed remote builds in B15114: Diff 20609.Jan 5 2023, 4:02 AM
tomek accepted this revision.Jan 5 2023, 7:57 AM
In D6052#185065, @przemek wrote:

Used scrollTo.
Added tests and run them.
There were some problems that I solved locally using fixes from here: https://linear.app/comm/issue/ENG-2654/problems-with-jest
I haven't pushed those fixes anywhere as it is to be discussed yet, but I found it acceptable to
push tests even if they fail currently (due to lack of mocks as mentioned on linear).
Similar thing is happening to tooltip-utils.test.js tests and it was ignored in the past I guess.

It's interesting, because it seems like the tests should've failed on the CI - but it doesn't seem so. Do we know if these were run at all?

This revision is now accepted and ready to land.Jan 5 2023, 7:57 AM
• przemek added a comment.Jan 5 2023, 9:24 AM

I think they're not. They also don't fail on local tests. I'm not sure why. When I run yarn test in web, tests in tooltip failed as well, and they were failing for a long time I guess.

Closed by commit rCOMMfb601172deb0: [web] Keyboard support for typeahead [12/13] - Scrolling thourgh overlay as… (authored by • przemek). · Explain WhyJan 12 2023, 6:21 AM
This revision was automatically updated to reflect the committed changes.
• przemek added a commit: rCOMMfb601172deb0: [web] Keyboard support for typeahead [12/13] - Scrolling thourgh overlay as….
ashoat added a comment.Jan 12 2023, 7:56 AM

Did we create a task for the tests not being run?

• przemek added a comment.EditedJan 13 2023, 2:15 AM

No, I'll do it right now

ashoat added a comment.Jan 13 2023, 6:32 AM

Task: https://linear.app/comm/issue/ENG-2712/we-dont-run-tests-in-web-native-and-desktop-in-precommit-hook

In the future, these tasks should be created (and linked) before landing the diff

Revision Contents
Changeset List

PathSize
web/
chat/
20 lines
utils/
25 lines
32 lines

Diff 20870

View Options

web/chat/typeahead-tooltip.react.js

Loading...
View Options

web/utils/typeahead-utils.js

Loading...
View Options

web/utils/typeahead-utils.test.js

Loading...