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
Unknown Object (File)
Wed, Apr 2, 4:21 AM
Unknown Object (File)
Mon, Mar 17, 9:22 AM
Unknown Object (File)
Mon, Mar 17, 9:22 AM
Unknown Object (File)
Mon, Mar 17, 9:22 AM
Unknown Object (File)
Mon, Mar 17, 9:22 AM
Unknown Object (File)
Mon, Mar 17, 9:22 AM
Unknown Object (File)
Mon, Mar 17, 9:21 AM
Unknown Object (File)
Mon, Mar 17, 9:13 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
No Lint Coverage
Unit
No Test Coverage

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

Maybe we should use scrollTo method?

web/utils/typeahead-utils.js
164–178

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/
19 lines
utils/
25 lines

Diff 20179

View Options

web/chat/typeahead-tooltip.react.js

  • This file was added.
div.containerViewerLiked {
padding: 4px;
border-radius: 4px;
background-color: var(--shades-black-80);
}
div.container:hover {
cursor: pointer;
}
tomekUnsubmitted
Not Done
Inline Actions

For other tooltip actions we don't need to manually set this style. Can we use the mechanism we already have, or are there any reasons for not doing so?

tomek: For other tooltip actions we don't need to manually set this style. Can we use the mechanism we…
ginsuAuthorUnsubmitted
Done
Inline Actions

I believe the other icons don't need to set this style because it is built into IcomoonReact. When I switched the Like action tooltip button with one of the icons the hover style just works out of the box. Wondering if @atul has more context since he is pretty familiar with the icons stuff

ginsu: I believe the other icons don't need to set this style because it is built into `IcomoonReact`.
tomekUnsubmitted
Not Done
Inline Actions

It wouldn't make too much sense for Icomoon to provide hover effects for icons because they could be used in other places - not only as buttons. It seems like the style on hover is applied in message-tooltip.css

div.messageActionButtons svg:hover {
  cursor: pointer;
  color: var(--fg);
}

so it should be enough to apply this style also to the new button.

tomek: It wouldn't make too much sense for `Icomoon` to provide hover effects for icons because they…
ginsuAuthorUnsubmitted
Done
Inline Actions

Ahhhhh that makes sense, thank you for clarifying, I will make the necessary changes

ginsu: Ahhhhh that makes sense, thank you for clarifying, I will make the necessary changes
View Options

web/utils/typeahead-utils.js

  • This file was added.
div.containerViewerLiked {
padding: 4px;
border-radius: 4px;
background-color: var(--shades-black-80);
}
div.container:hover {
cursor: pointer;
}
tomekUnsubmitted
Not Done
Inline Actions

For other tooltip actions we don't need to manually set this style. Can we use the mechanism we already have, or are there any reasons for not doing so?

tomek: For other tooltip actions we don't need to manually set this style. Can we use the mechanism we…
ginsuAuthorUnsubmitted
Done
Inline Actions

I believe the other icons don't need to set this style because it is built into IcomoonReact. When I switched the Like action tooltip button with one of the icons the hover style just works out of the box. Wondering if @atul has more context since he is pretty familiar with the icons stuff

ginsu: I believe the other icons don't need to set this style because it is built into `IcomoonReact`.
tomekUnsubmitted
Not Done
Inline Actions

It wouldn't make too much sense for Icomoon to provide hover effects for icons because they could be used in other places - not only as buttons. It seems like the style on hover is applied in message-tooltip.css

div.messageActionButtons svg:hover {
  cursor: pointer;
  color: var(--fg);
}

so it should be enough to apply this style also to the new button.

tomek: It wouldn't make too much sense for `Icomoon` to provide hover effects for icons because they…
ginsuAuthorUnsubmitted
Done
Inline Actions

Ahhhhh that makes sense, thank you for clarifying, I will make the necessary changes

ginsu: Ahhhhh that makes sense, thank you for clarifying, I will make the necessary changes