Page MenuHomePhabricator

[web] introduced thread-picker-modal.react
ClosedPublic

Authored by ginsu on Oct 7 2022, 11:04 AM.
Tags
None
Referenced Files
Unknown Object (File)
Mon, Apr 22, 9:49 AM
Unknown Object (File)
Thu, Apr 11, 5:58 AM
Unknown Object (File)
Mon, Apr 8, 1:35 AM
Unknown Object (File)
Mon, Apr 8, 1:35 AM
Unknown Object (File)
Mon, Apr 8, 1:35 AM
Unknown Object (File)
Mon, Apr 8, 1:35 AM
Unknown Object (File)
Mon, Apr 8, 1:34 AM
Unknown Object (File)
Mon, Apr 8, 1:15 AM

Details

Summary

introduced thread-picker-modal.react component. This diff just introduces the component, but it is not used anywhere at the moment

Next steps:

  1. Implement ThreadPickerModal component
  2. Clean up/get rid of old ThreadPicker component

Linear Issue: ENG-1848

Design: ENG-1778

Test Plan

Please see the video to see how the adding calendar event interaction now works with this new component.

To test interaction on local machine:

Replace this.setState({ pickerOpen: true }); on line 213 of web/calendar/day.react.js

this.props.pushModal(
  <ThreadPickerModal
    name="Chats"
    onClose={this.props.popModal}
    createNewEntry={this.createNewEntry}
  />,
);

import ThreadPickerModal with import ThreadPickerModal from '../modals/threads/thread-picker-modal.react';

And add the popModal function by adding this code in the ConnectedDay component and Day component props:

Screen Shot 2022-10-07 at 1.58.49 PM.png (1×1 px, 294 KB)

Screen Shot 2022-10-07 at 1.59.16 PM.png (398×944 px, 85 KB)

Diff Detail

Repository
rCOMM Comm
Lint
Lint Not Applicable
Unit
Tests Not Applicable

Event Timeline

ginsu requested review of this revision.Oct 7 2022, 11:15 AM
web/modals/threads/thread-picker-modal.react.js
56–98 ↗(On Diff #17418)

All this logic was taken from D5289

Looks good! Minor nits inline

web/modals/threads/thread-picker-modal.css
25 ↗(On Diff #17418)

nit: We call it "splotch" elsewhere in the codebase:

4ca76f.png (1×1 px, 1019 KB)

so we could probably rename this to threadSplotch?

web/modals/threads/thread-picker-modal.react.js
24 ↗(On Diff #17418)

Can we name this something a little more specific than onClick to make the purpose of this function a bit clearer?

25–26 ↗(On Diff #17418)

Thoughts on swapping these two function calls positionally?

29–34 ↗(On Diff #17418)

Nice, good catch memoizing this style object!


(Same note as above but something like splotchColorStyle would be more consistent with what's already in the codebase)

56–98 ↗(On Diff #17418)

Thanks for linking the diff where this logic was previously reviewed/approved

112 ↗(On Diff #17418)

Good call displaying "No results" if there aren't any results... makes it clear to the user that we're not waiting to load them or anything

This revision is now accepted and ready to land.Oct 7 2022, 1:08 PM
ginsu edited the summary of this revision. (Show Details)

resovled atul's comments

fixed typo with css class name

This revision was automatically updated to reflect the committed changes.
web/modals/threads/thread-picker-modal.react.js
79

Defining this createSelector inline in a function component makes zero sense. The whole point of createSelector is its caching behavior... but this createSelector gets redefined on every render, so no caching is achieved

93–99

This useSelector makes even less sense! It's not even looking at the Redux state... hard to imagine how this made sense to either @ginsu or @atul

(I'll handle this, already working on it)

web/modals/threads/thread-picker-modal.react.js
93–99

Ah dang, totally should've caught this... think this was before @ginsu even joined full time so definitely on me.