Page MenuHomePhabricator

[web] Typeahead overlay visual
ClosedPublic

Authored by przemek on Nov 17 2022, 2:36 AM.
Tags
None
Referenced Files
F3382485: D5660.diff
Thu, Nov 28, 11:02 AM
Unknown Object (File)
Sun, Nov 24, 8:59 PM
Unknown Object (File)
Sun, Nov 24, 8:58 PM
Unknown Object (File)
Sun, Nov 24, 7:59 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
Unknown Object (File)
Fri, Nov 22, 7:59 AM

Details

Summary

Component that will be displayed when user tries to mention other user (by typing @ in chatbar).
Currently, it is not created anywhere, but code that does it will be added in next diffs.
Component takes list of actions and it's absolute position that will be calculated outside of it.
Notice it isn't positioned correctly yet, as it's work in progress.

Relevant linear tasks:
https://linear.app/comm/issue/ENG-2096/typeahead-in-tooltip-for-mentions
https://linear.app/comm/issue/DES-1/design-typeahead-for-mentions-on-web#comment-f40bb8c5

Test Plan

I already have most of the outer logic completed, so I was able to test if it's rendered properly.
Video below:

Diff Detail

Repository
rCOMM Comm
Lint
No Lint Coverage
Unit
No Test Coverage

Event Timeline

Added theme.css changes as well. Sorry for missing that.

web/chat/mention-suggestion-tooltip.react.js
1 ↗(On Diff #18517)

Nit: add a newline, please match conventions in codebase as much as possible

atul requested changes to this revision.Nov 17 2022, 4:42 PM

Minor suggestions inline

web/chat/mention-suggestion-tooltip.css
12 ↗(On Diff #18517)

Good catch using CSS variables here for color value!

15 ↗(On Diff #18517)

Can we move this color to web/theme.css and consume CSS variable from here?

web/chat/mention-suggestion-tooltip.react.js
27–34 ↗(On Diff #18517)

We don't need to memoize this because classNames returns a string.

(Further reading: https://blog.isquaredsoftware.com/2020/05/blogged-answers-a-mostly-complete-guide-to-react-rendering-behavior/)

36–42 ↗(On Diff #18517)

We, however, do need to memorize things here since we're returning a style object.

54 ↗(On Diff #18517)

We can probably drop this explicit else to reduce indentation

This revision now requires changes to proceed.Nov 17 2022, 4:42 PM
web/chat/mention-suggestion-tooltip.react.js
36–42 ↗(On Diff #18517)
przemek marked 4 inline comments as done.

Responded to inlines, fixed code and tested if it didn't break renders.

web/chat/mention-suggestion-tooltip.react.js
27–34 ↗(On Diff #18517)

Right, thanks for source. I think we should create linear task for other occurrences of those in our codebase, as sometimes we use useMemo and sometimes we don't
{F251446}

tomek requested changes to this revision.Nov 18 2022, 4:38 AM
tomek added inline comments.
web/chat/mention-suggestion-tooltip.css
20 ↗(On Diff #18562)

Does this handle longer usernames correctly (by adding an ellipsis)?

34–37 ↗(On Diff #18562)

Why do we set these? Especially order: 0 is concerning.

web/chat/mention-suggestion-tooltip.react.js
10 ↗(On Diff #18562)

It's more convenient to use mixed as a return type. It allows passing any function instead of just the void ones.

30 ↗(On Diff #18562)

Can't we use a simpler approach? We have max-height set so adding overflow: auto should work just fine.

35 ↗(On Diff #18562)

We can simply set this position in css file

44 ↗(On Diff #18562)

If we're setting onClick handler, we should avoid using div - we should try to use more semantic html. In this case button sounds appropriate.

This revision now requires changes to proceed.Nov 18 2022, 4:38 AM
przemek marked 6 inline comments as done.

Responded to inlines, fixed where neccessary.

Fixed button import...
Fallen into a trap of VSCode automatically importing Button as button.

web/chat/mention-suggestion-tooltip.css
20 ↗(On Diff #18562)

It is troublesome to add that in flex container, but I added span element wrapping text inside and set text-overflow on it.

34–37 ↗(On Diff #18562)

To be honest, I don't have much experience with Figma and copy-pasted all styles, only later realising some of them are unnecessary. Refactored it.

web/chat/mention-suggestion-tooltip.react.js
44 ↗(On Diff #18562)

Done, but I had to add some styles to cancel out default button styles.

web/chat/mention-suggestion-tooltip.css
34–37 ↗(On Diff #18562)

Ah yeah I'd be super careful about copying styles from Figma.

From what I understand they somehow translate their "auto-layout" system to CSS in a way that sometimes works great(?), but often spits out a bunch of weird stuff.

I think for the CSS to be cleanly export-able it would take extra work from the designers to use "auto-layout" in some precise/specific/limited way.

I don't even fully trust Figma SVG export given past experience...

web/chat/mention-suggestion-tooltip.css
34–37 ↗(On Diff #18562)

As a general rule, you should understand every single code of code you submit for review. If you're copy-pasting from somewhere, that means going line-by-line and researching what that line does and deciding if we need it

atul added inline comments.
web/chat/mention-suggestion-tooltip.css
16 ↗(On Diff #18572)

Personally don't love numbers in variable names, could we do something like typeahead-overlay-shadow-primary and typeahead-overlay-shadow-secondary?

Really doesn't matter and I don't think we have a team convention or rule or anything... so whatever you prefer

(We do have numbers for some of the font CSS variables eg --m-font-16 so we've allowed it in the past)

tomek added inline comments.
web/chat/mention-suggestion-tooltip.css
42–44 ↗(On Diff #18572)

Do we need to set all of these?

46 ↗(On Diff #18572)

We should use a variable --line-height-text

This revision is now accepted and ready to land.Nov 21 2022, 4:04 AM
przemek marked 3 inline comments as done.

Responded to inlines

web/chat/mention-suggestion-tooltip.css
16 ↗(On Diff #18572)

Good point, I didn't like it either, but didn't really know, what to call these variables. I like you suggestion

42–44 ↗(On Diff #18572)

From what I've seen in Dev tools and in this thread: https://stackoverflow.com/questions/26140050/why-is-font-family-not-inherited-in-button-tags-automatically
<button> tag doesn't inherit font styles. If I don't set them here, button texts are rendered incorrectly according to design.

This revision was automatically updated to reflect the committed changes.