[web] refactored ThreadPicker from class based to functional
ClosedPublic
Actions

Authored by ginsu on Oct 3 2022, 12:30 PM.

Details

Reviewers

atul
rohan
• abosh
tomek

Commits

rCOMMd6fb44cfa346: [web] refactored ThreadPicker from class based to functional

Summary

refactored the ThreadPicker component to be a functional component now. The purpose of this was to optimize the searching and filtering of threads through the useMemo hook. Again there is no UI for this diff due to Search breaking the app since the focusing/blurring of the picker component prevents input in Search

ENG-1914

Depends on D5239

Test Plan

To test the search feature, please modify the return function:

return (
  <div
    className={css.container}
    tabIndex="0"
    ref={pickerDivRef}
    // onBlur={closePicker}
    onKeyDown={onPickerKeyDown}
    // onMouseDown={onMouseDown}
  >
    <Search
      searchText={searchText}
      onChangeText={onChangeSearchText}
      placeholder="Search chats"
    />
    {options}
  </div>
);

You will also need to import the Search component from ../components/search.react at the top of the file

Once you have the Search component being returned you can add a new entry in the calendar and test that the search feature is working as expected

Diff Detail

Repository

rCOMM Comm

Lint

Lint Not Applicable

Unit

Tests Not Applicable

Event Timeline

ginsu created this revision.Oct 3 2022, 12:30 PM

Herald added subscribers: • abosh, atul, tomek, ashoat. · View Herald TranscriptOct 3 2022, 12:30 PM

ginsu edited the summary of this revision. (Show Details)Oct 3 2022, 12:35 PM

ginsu edited the test plan for this revision. (Show Details)

ginsu added reviewers: atul, rohan, • abosh.

ginsu added a parent revision: D5239: [web] implemented search feature for Thread Picker.

Harbormaster completed remote builds in B12588: Diff 17301.Oct 3 2022, 12:41 PM

ginsu requested review of this revision.Oct 3 2022, 12:41 PM

Looks close, left some notes/questions/suggestions inline

web/calendar/thread-picker.react.js
50 ↗	(On Diff #17301)	Is there a reason we're flipping the case here? Think it should still be `onScreenThreadInfos`?
55 ↗	(On Diff #17301)	Can you explain why we need to use `useLayoutEffect` here? It looks like we do something similar in `log-in-form.react.js` `thread-settings-delete-tab.react.js` `thread-settings-general-tab.react.js` Is there something different about this situation that I'm missing?
56 ↗	(On Diff #17301)	I think something like `pickerDivRef must be set` would be more inline with existing `invariant` messages in the codebase.
79 ↗	(On Diff #17301)	Similarly, something like `pickerDivRef must be set` would be more consistent with `invariant` messages that are already in the codebase.
98–99 ↗	(On Diff #17301)	Can we use the `useSelector` hook instead of memoizing what gets returned from `createSelector`?

This revision now requires changes to proceed.Oct 3 2022, 4:28 PM

resovled atul's comments

Harbormaster completed remote builds in B12598: Diff 17321.Oct 4 2022, 8:26 AM

ginsu added inline comments.Oct 4 2022, 8:30 AM

web/calendar/thread-picker.react.js
55–59 ↗	(On Diff #17321)	Spoke to @atul about this IRL, but I was taught that we use `useLayoutEffect` whenever we use `useRef` and we want to manipulate the DOM (focusing) Here are some source: Source 1 Source 2 If we do decide to go with this, I can also create a new task to switch the `useEffect` to `useLayoutEffect` in these files that @atul mentioned previously `log-in-form.react.js` `thread-settings-delete-tab.react.js` `thread-settings-general-tab.react.js`
88 ↗	(On Diff #17321)	This will be removed in future diff when `Search` component calls this method
98–110 ↗	(On Diff #17321)	If we decide to go with this, I will create an additional task in the backlog to eventually put this in `lib/selectors`