Page MenuHomePhabricator
Differential D6045

[web] Keyboard support for typeahead [5/13] - Moved finding suggested users out of tooltip component
ClosedPublic
Actions

Authored by • przemek on Dec 27 2022, 4:45 AM.
Tags
None
Referenced Files
Unknown Object (File)
Wed, Apr 2, 6:24 AM
Unknown Object (File)
Wed, Apr 2, 4:21 AM
Unknown Object (File)
Mon, Mar 31, 8:27 AM
Unknown Object (File)
Thu, Mar 20, 1:30 AM
Unknown Object (File)
Mon, Mar 17, 9:21 AM
Unknown Object (File)
Mon, Mar 17, 9:21 AM
Unknown Object (File)
Mon, Mar 17, 9:21 AM
Unknown Object (File)
Mon, Mar 17, 9:21 AM
View All Files
Subscribers
ashoat
atul
ginsu
inka
kamil
tomek

Details

Reviewers
inka
kamil
ginsu
tomek
Commits
rCOMMd7f2decc2753: [web] Keyboard support for typeahead [5/13] - Moved finding suggested users out…
Summary

I moved logic user search out of tooltip component. It allows to simplify component and its rendering logic.
Now we only show overlay if we find any users in thread matching typed text.

Test Plan

Typeahead without keyboard support works.
Final tests performed in last diffs.

Diff Detail

Repository
rCOMM Comm
Lint
Lint Not Applicable
Unit
Tests Not Applicable

Event Timeline

• przemek created this revision.Dec 27 2022, 4:45 AM
Herald added subscribers: atul, ashoat. · View Herald TranscriptDec 27 2022, 4:45 AM
Harbormaster completed remote builds in B14778: Diff 20156.Dec 27 2022, 4:59 AM
• przemek requested review of this revision.Dec 27 2022, 4:59 AM
• przemek added a parent revision: D6044: [web] Keyboard support for typeahead [4/13] - Renamed regex.Dec 27 2022, 5:30 AM
• przemek added a child revision: D6046: [web] Keyboard support for typeahead [6/13] - Get rid of entire text and rename matched strings.
ashoat added inline comments.Dec 27 2022, 12:27 PM
web/chat/chat-input-bar.react.js
562–571 ↗(On Diff #20156)

Early exit always please. Please keep this in mind going forward!

• przemek updated this revision to Diff 20387.Dec 29 2022, 12:04 PM

Returned early.

Harbormaster completed remote builds in B14946: Diff 20387.Dec 29 2022, 12:18 PM
inka accepted this revision.Dec 30 2022, 2:10 AM
inka added a reviewer: tomek.
tomek accepted this revision.Dec 30 2022, 8:33 AM

Nice!

This revision is now accepted and ready to land.Dec 30 2022, 8:33 AM
Closed by commit rCOMMd7f2decc2753: [web] Keyboard support for typeahead [5/13] - Moved finding suggested users out… (authored by • przemek). · Explain WhyJan 12 2023, 6:21 AM
This revision was automatically updated to reflect the committed changes.
• przemek added a commit: rCOMMd7f2decc2753: [web] Keyboard support for typeahead [5/13] - Moved finding suggested users out….

Revision Contents
Changeset List

PathSize
lib/
shared/
13 lines
web/
chat/
35 lines
30 lines

Diff 20863

View Options

lib/shared/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
View Options

web/chat/chat-input-bar.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/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