Page MenuHomePhabricator
Differential D13995

[native] introduce community list component
AcceptedPublic
Actions

Authored by varun on Nov 21 2024, 1:41 PM.
Tags
None
Referenced Files
F3514833: D13995.id46370.diff
Sun, Dec 22, 6:39 AM
F3514832: D13995.id45932.diff
Sun, Dec 22, 6:39 AM
F3514809: D13995.id.diff
Sun, Dec 22, 6:39 AM
F3514797: D13995.diff
Sun, Dec 22, 6:38 AM
Unknown Object (File)
Mon, Dec 16, 1:48 PM
Unknown Object (File)
Tue, Dec 10, 4:51 AM
Unknown Object (File)
Sat, Nov 30, 6:05 PM
Unknown Object (File)
Sat, Nov 30, 6:01 PM
View All 19 Files
Subscribers
tomek

Details

Reviewers
ashoat
Summary

this component is basically a fork of the native ThreadList component

the differences are, the new component:

  • renders CommunityListItems
  • does not have an onSelect (we may consider adding this in the future to display community info)
  • does not have a searchRef (we don't want to call focus() on the textInput, keyboard should not display unless user presses search bar)

Depends on D13994

Test Plan

see video

Diff Detail

Repository
rCOMM Comm
Lint
No Lint Coverage
Unit
No Test Coverage

Event Timeline

varun created this revision.Nov 21 2024, 1:41 PM
Herald added a subscriber: tomek. · View Herald TranscriptNov 21 2024, 1:41 PM
varun edited the test plan for this revision. (Show Details)Nov 21 2024, 1:44 PM
varun attached a referenced file: F3336053: RPReplay_Final1732225392.MP4. (Show Details)
varun mentioned this in D13994: [native] community list item component.
Harbormaster completed remote builds in B32756: Diff 45932.Nov 21 2024, 2:00 PM
varun requested review of this revision.Nov 21 2024, 2:00 PM
ashoat requested changes to this revision.Nov 21 2024, 6:28 PM

React class components should not be used for new components in 95% of cases... can you try to make this a function component?

native/components/community-list.react.js
44 ↗(On Diff #45932)

We should not be introducing any new class components

50–65 ↗(On Diff #45932)

This messy pattern is approximating React.useMemo

84 ↗(On Diff #45932)

There's a shorthand for this now

95 ↗(On Diff #45932)
This revision now requires changes to proceed.Nov 21 2024, 6:28 PM
varun added a child revision: D13996: [native] community joiner modal.Nov 21 2024, 7:58 PM
varun updated this revision to Diff 46370.Wed, Dec 11, 8:43 PM

rewrite as a functional component

Harbormaster completed remote builds in B33052: Diff 46370.Wed, Dec 11, 9:00 PM
ashoat accepted this revision.Thu, Dec 12, 10:49 AM
ashoat added inline comments.
native/components/community-list.react.js
24–26

Do we actually have multiple use cases for this component?

If not, I think:

  1. We should avoid having optional properties, and make them all requires
  2. We should remove any properties that are not used in our upcoming first (and only?) use case
48–50

Nit: shorthand

This revision is now accepted and ready to land.Thu, Dec 12, 10:49 AM

Revision Contents
Changeset List

PathSize
native/
components/
108 lines

Diff 46370

View Options

native/components/community-list.react.js

Loading...