Page MenuHomePhabricator

[web] Add drawer item implementations

Authored by inka on Jan 4 2023, 5:55 AM.
Referenced Files
Unknown Object (File)
Sun, Mar 5, 8:03 AM
Unknown Object (File)
Sat, Feb 25, 1:00 AM
Unknown Object (File)
Fri, Feb 24, 6:44 PM
Unknown Object (File)
Fri, Feb 24, 7:16 AM
Unknown Object (File)
Feb 20 2023, 8:16 PM
Unknown Object (File)
Feb 9 2023, 12:19 PM
F353564: image.png
Jan 31 2023, 7:09 AM
Unknown Object (File)
Jan 16 2023, 8:52 PM



Linear issue:
Adding drawer item implementations on native: D5721
There are two types of drawer items - [community] and [a thread in a community]. They behave almost the same, but there is a difference when their expand buttons are toggled: only one community can
be open, so opening a community sometimes has to trigger closing another. On the other hand multiple level 1 and level 2 threads can be open at once.
The information about which community is open is thus held by a parent component, and a community is passed an expanded flag and a setExpanded function allowing it to change its parents state.
This is why I created CommunityDrawerItemCommunity and CommunityDrawerItemChat components.

Test Plan

Added community drawer item to ChatThreadListItem, checked that pressinng it navigates to the corresponding thread, and pressinng its subchannels button navigates to the corresponding subchannels

Diff Detail

rCOMM Comm
Lint Not Applicable
Tests Not Applicable

Event Timeline

200 ↗(On Diff #20571)

This value is not from our design system because

  1. For now the designs are in progress, so I left colours from native, as they match what is now on web.
  2. This value was set in figma as shades of white 60 and multiply, so I just calculated the right colour.

This will be likely changed anyway when the designs for web are done.

inka requested review of this revision.Jan 4 2023, 6:10 AM
25–38 ↗(On Diff #20571)

These are styles that will be used by item labels of different levels. They will be passed in labelStyle of itemData for each item.

Additionally, in a few places, we could use margin and padding shorthand properties instead of margin-top etc. But I'm not that experienced in css so I'm not sure which is preferred.

1 ↗(On Diff #20571)

I don't think we need to specify the type.

44–51 ↗(On Diff #20571)

We can simplify this

89 ↗(On Diff #20571)

Can labelStyle be null? If not can it be simplified to classnames(css.title, css[labelStyle])?

105–131 ↗(On Diff #20571)

Maybe we could move it to the seperate file like CommunityDrawerItemCommunity

105–131 ↗(On Diff #20571)

We cannot. Detailed explanation is in D5721 summary and comments.

Address code review + changes due to changes in previous diffs

51–62 ↗(On Diff #20674)

Move those to seperate file so we follow convention: css file per react.js file that it styles.

57 ↗(On Diff #20674)

Personally I found find it easier to read and understand if we explicitly compare with 0 instead of casting to boolean

75 ↗(On Diff #20674)

That name may be a bit misleading as we would expect onClick function to be a callback to an event, but here it's called inside of actual callback. I can see you were inspired by naming in our codebase, but I think going with something like navigateToThread would make code more readable

91–93 ↗(On Diff #20674)

Why did we use <a> instead of <button> or <Button> component?

104–122 ↗(On Diff #20674)

I am no expert on React, but it feels like this component is unnecessary. Only thing it does is introducing state, and state toggling function and passing it to CommunityDrawerItem which later on creates more CommunityDrawerItemChat components.

Wouldn't better solution be getting rid of CommunityDrawerItemChat entirely? We would move expandeed state to CommunityDrawerItem, which is the only thing that uses and changes that anyway. It creates confusion when it comes to naming, and I think we can create CommunityDrawerItem components recursively inside CommunityDrawerItem.
What do you think? What would be cons of such solution?

115–119 ↗(On Diff #20674)

Shouldn't we use MemoizedCommunityDrawerItem here instead?

tomek requested changes to this revision.Jan 31 2023, 6:51 AM

A couple of nits but overall looks ok

22 ↗(On Diff #20674)

Can we use a constant e.g. --line-height-text? Or is it independent from font size?

30–38 ↗(On Diff #20674)

You can merge these

5 ↗(On Diff #20674)

We should use utils/redux-utils instead

52–54 ↗(On Diff #20674)

This can be simplified

78 ↗(On Diff #20674)

Are you sure that we want an or?

This revision now requires changes to proceed.Jan 31 2023, 6:51 AM
51–62 ↗(On Diff #20674)

I don't think we follow that convention

image.png (632×1 px, 122 KB)

And personally since all these styles relate to drawer items, and there aren't many of the ones used in a different file, this just seems cleaner.

75 ↗(On Diff #20674)

This will anyways be overridden by D6292, so if you don't mind I'll leave it to avoid unnecessary conflicts. Since it's just a name change

91–93 ↗(On Diff #20674)

We talked about this with @michal (author of <Button>) and @przemek, and we all agreed that this code is correct

104–122 ↗(On Diff #20674)

We talked about this offline, and agreed that this code makes sense.
For detailed explanation see D5721

115–119 ↗(On Diff #20674)

Actually I don't think it matters, since whenever CommunityDrawerItemChat re-renders, that means it's props have changed (since we only use it's memoized version), so props for CommunityDrawerItem/MemoizedCommunityDrawerItem it renders also change. But it won't hurt

Address code review

22 ↗(On Diff #20674)

It's hard to tell, the designs keep changing. In the designs that I based this on, it was indeed independent from the font size.
I'll revisit this during the styling task, because for example in the newest designs all labels are of the same size, so all of this code will be changed.

78 ↗(On Diff #20674)

This code is copied from chat-thread-list-item.react.js, and it does the same thing as in ChatThreadListItem

In D6292 we talked about this, and

  1. it gets overridden there, so it doesn't really matter
  2. then if we were creating a chat and pressed on a different item, we wouldn't navigate to it. Since if isCreateMode === true, then the condition wouldn't be met, and onClick wouldn't be called. And I don't think that's what we want to happen
  3. We might want to not navigate again to a chat that is already chosen (even if it's not just being created), but that should be changed in ChatThreadListItem, and we have a task for this
8 ↗(On Diff #21683)

I checked and in other places on web we use useSelector from ../redux/redux-utils. If I should use useSelector from utils/redux-utils anyway, please let me know

tomek added inline comments.
8 ↗(On Diff #21683)

You're right!

This revision is now accepted and ready to land.Feb 7 2023, 7:14 AM