Page MenuHomePhabricator

[web] Add drawer item implementations
ClosedPublic

Authored by inka on Jan 4 2023, 5:55 AM.
Tags
None
Referenced Files
F3357571: D6160.diff
Sun, Nov 24, 12:19 AM
Unknown Object (File)
Mon, Nov 4, 9:24 AM
Unknown Object (File)
Sat, Nov 2, 7:58 AM
Unknown Object (File)
Mon, Oct 28, 1:57 AM
Unknown Object (File)
Sun, Oct 27, 8:57 PM
Unknown Object (File)
Oct 20 2024, 4:11 AM
Unknown Object (File)
Oct 20 2024, 4:10 AM
Unknown Object (File)
Oct 19 2024, 4:28 AM
Subscribers

Details

Summary

Linear issue: https://linear.app/comm/issue/ENG-2631/drawer-sidebar-inside-of-community-picker
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
modal.

Diff Detail

Repository
rCOMM Comm
Branch
inka/community_drawer_web
Lint
No Lint Coverage
Unit
No Test Coverage

Event Timeline

web/theme.css
200

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
web/sidebar/community-drawer-item.css
25–38

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.

web/sidebar/community-drawer-item.css
1

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

web/sidebar/community-drawer-item.react.js
44–51

We can simplify this

89

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

web/sidebar/community-drawer-item.react.js
105–131

Maybe we could move it to the seperate file like CommunityDrawerItemCommunity

web/sidebar/community-drawer-item.react.js
105–131

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

Address code review + changes due to changes in previous diffs

web/sidebar/community-drawer-item.css
51–62 ↗(On Diff #20674)

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

web/sidebar/community-drawer-item.react.js
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

web/sidebar/community-drawer-item.css
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

web/sidebar/community-drawer-item.react.js
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
web/sidebar/community-drawer-item.css
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.

web/sidebar/community-drawer-item.react.js
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

web/sidebar/community-drawer-item.css
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.

web/sidebar/community-drawer-item.react.js
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
web/sidebar/community-drawer-item.react.js
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.
web/sidebar/community-drawer-item.react.js
8 ↗(On Diff #21683)

You're right!

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