Changeset View
Standalone View
web/modals/threads/create/compose-subchannel-modal.react.js
- This file was added.
// @flow | ||||||||||||||||||||||||||||
import * as React from 'react'; | ||||||||||||||||||||||||||||
import type { ThreadInfo } from 'lib/types/thread-types'; | ||||||||||||||||||||||||||||
import { trimText } from 'lib/utils/text-utils'; | ||||||||||||||||||||||||||||
import Stepper from '../../../components/stepper.react'; | ||||||||||||||||||||||||||||
import Modal from '../../modal.react'; | ||||||||||||||||||||||||||||
import css from './compose-subchannel-modal.css'; | ||||||||||||||||||||||||||||
import SubchannelMembers from './steps/subchannel-members.react'; | ||||||||||||||||||||||||||||
import SubchannelSettings from './steps/subchannel-settings.react'; | ||||||||||||||||||||||||||||
type Props = { | ||||||||||||||||||||||||||||
+onClose: () => any, | ||||||||||||||||||||||||||||
atul: We want to avoid typing things as `any`. Can we type this as `mixed` or `void` instead? | ||||||||||||||||||||||||||||
tomekUnsubmitted Done Inline ActionsIn callback return value mixed is preferred because it allows using any function instead of only the ones that return nothing. tomek: In callback return value `mixed` is preferred because it allows using any function instead of… | ||||||||||||||||||||||||||||
jakubAuthorUnsubmitted Done Inline ActionsI had to use void type, because onClose props in Modal component requires it. Otherwise, flow throws this error: Cannot create `Modal` element because mixed [1] is incompatible with undefined [2] in the return value of property `onClose` jakub: I had to use `void` type, because `onClose` props in `Modal` component requires it. Otherwise… | ||||||||||||||||||||||||||||
+parentThreadInfo: ThreadInfo, | ||||||||||||||||||||||||||||
}; | ||||||||||||||||||||||||||||
type Pages = 'settings' | 'members'; | ||||||||||||||||||||||||||||
type VisibilityType = 'open' | 'closed'; | ||||||||||||||||||||||||||||
type HeaderProps = { | ||||||||||||||||||||||||||||
+parentThreadName: string, | ||||||||||||||||||||||||||||
}; | ||||||||||||||||||||||||||||
function ComposeSubchannelHeader(props: HeaderProps): React.Node { | ||||||||||||||||||||||||||||
const { parentThreadName } = props; | ||||||||||||||||||||||||||||
return ( | ||||||||||||||||||||||||||||
<div className={css.modalHeader}> | ||||||||||||||||||||||||||||
<div> | ||||||||||||||||||||||||||||
{'within '} | ||||||||||||||||||||||||||||
<div className={css.modalHeaderParentName}>{parentThreadName}</div> | ||||||||||||||||||||||||||||
</div> | ||||||||||||||||||||||||||||
</div> | ||||||||||||||||||||||||||||
); | ||||||||||||||||||||||||||||
} | ||||||||||||||||||||||||||||
function ComposeSubchannelModal(props: Props): React.Node { | ||||||||||||||||||||||||||||
const { parentThreadInfo } = props; | ||||||||||||||||||||||||||||
const { uiName: parentThreadName } = parentThreadInfo; | ||||||||||||||||||||||||||||
const [activeStep, setActiveStep] = React.useState<Pages>('settings'); | ||||||||||||||||||||||||||||
atulUnsubmitted Done Inline ActionsMinor nit: Could we name this activePage or change the name of Pages => Steps to keep things symmetrical? atul: Minor nit: Could we name this `activePage` or change the name of `Pages` => `Steps` to keep… | ||||||||||||||||||||||||||||
const [channelName, setChannelName] = React.useState<string>(''); | ||||||||||||||||||||||||||||
const [visibilityType, setVisibilityType] = React.useState<VisibilityType>( | ||||||||||||||||||||||||||||
'open', | ||||||||||||||||||||||||||||
); | ||||||||||||||||||||||||||||
const [announcement, setAnnouncement] = React.useState<boolean>(false); | ||||||||||||||||||||||||||||
const [selectedUsers, setSelectedUsers] = React.useState<Set<string>>( | ||||||||||||||||||||||||||||
tomekUnsubmitted Done Inline ActionsNot really sure, but probably using $ReadOnlySet would make selectedUsers readonly. Can you check that? tomek: Not really sure, but probably using `$ReadOnlySet` would make `selectedUsers` readonly. Can you… | ||||||||||||||||||||||||||||
jakubAuthorUnsubmitted Done Inline ActionsSure, I check that and it could be $ReadOnlySet. I set this temporary and I missed it. I will change that jakub: Sure, I check that and it could be `$ReadOnlySet`. I set this temporary and I missed it. I will… | ||||||||||||||||||||||||||||
new Set(), | ||||||||||||||||||||||||||||
); | ||||||||||||||||||||||||||||
const [searchUserText, setSearchUserText] = React.useState<string>(''); | ||||||||||||||||||||||||||||
const handleChanges = React.useCallback( | ||||||||||||||||||||||||||||
( | ||||||||||||||||||||||||||||
event: SyntheticEvent<HTMLInputElement>, | ||||||||||||||||||||||||||||
setValue: (newValue: any) => any, | ||||||||||||||||||||||||||||
) => { | ||||||||||||||||||||||||||||
const target = event.currentTarget; | ||||||||||||||||||||||||||||
setValue(target.value); | ||||||||||||||||||||||||||||
}, | ||||||||||||||||||||||||||||
[], | ||||||||||||||||||||||||||||
); | ||||||||||||||||||||||||||||
atulUnsubmitted Done Inline ActionsI think this introduces a confusing step of indirection. As far as I can tell (at this point in the stack), this is only used in onChangeChannelName(...). Can we move this logic inside onChangeChannelName(...) to make things clearer? Let me know if I'm missing some context here atul: I think this introduces a confusing step of indirection.
As far as I can tell (at this point… | ||||||||||||||||||||||||||||
jakubAuthorUnsubmitted Done Inline ActionsI prepared this function at the beginning of working on this component. At that moment I used it for changing channel type in two another functions, but finally I use another approach. So, we can move it inside onChangeChannelName jakub: I prepared this function at the beginning of working on this component. At that moment I used… | ||||||||||||||||||||||||||||
const onChangeChannelName = React.useCallback( | ||||||||||||||||||||||||||||
(event: SyntheticEvent<HTMLInputElement>) => { | ||||||||||||||||||||||||||||
handleChanges(event, setChannelName); | ||||||||||||||||||||||||||||
}, | ||||||||||||||||||||||||||||
[handleChanges], | ||||||||||||||||||||||||||||
); | ||||||||||||||||||||||||||||
const onOpenVisibilityTypeSelected = React.useCallback( | ||||||||||||||||||||||||||||
() => setVisibilityType('open'), | ||||||||||||||||||||||||||||
[], | ||||||||||||||||||||||||||||
); | ||||||||||||||||||||||||||||
const onClosedVisibilityTypeSelected = React.useCallback( | ||||||||||||||||||||||||||||
() => setVisibilityType('closed'), | ||||||||||||||||||||||||||||
[], | ||||||||||||||||||||||||||||
); | ||||||||||||||||||||||||||||
const onAnnouncementSelected = React.useCallback( | ||||||||||||||||||||||||||||
() => setAnnouncement(!announcement), | ||||||||||||||||||||||||||||
[announcement], | ||||||||||||||||||||||||||||
); | ||||||||||||||||||||||||||||
const switchUser = React.useCallback((userID: string) => { | ||||||||||||||||||||||||||||
atulUnsubmitted Done Inline ActionsWhat do you think about changing the name here from switchUser to something like toggleUserSelected or toggleUserSelection? I think it might make things a little clearer atul: What do you think about changing the name here from `switchUser` to something like… | ||||||||||||||||||||||||||||
jakubAuthorUnsubmitted Done Inline ActionsI tried to keep conventions like in AddMembersList component, but I could change it. jakub: I tried to keep conventions like in `AddMembersList` component, but I could change it. | ||||||||||||||||||||||||||||
setSelectedUsers((users: Set<string>) => { | ||||||||||||||||||||||||||||
const newUsers = new Set(users); | ||||||||||||||||||||||||||||
if (newUsers.has(userID)) { | ||||||||||||||||||||||||||||
newUsers.delete(userID); | ||||||||||||||||||||||||||||
} else { | ||||||||||||||||||||||||||||
newUsers.add(userID); | ||||||||||||||||||||||||||||
} | ||||||||||||||||||||||||||||
return newUsers; | ||||||||||||||||||||||||||||
}); | ||||||||||||||||||||||||||||
}, []); | ||||||||||||||||||||||||||||
const steps = []; | ||||||||||||||||||||||||||||
const subchannelSettings = React.useMemo( | ||||||||||||||||||||||||||||
() => ( | ||||||||||||||||||||||||||||
<SubchannelSettings | ||||||||||||||||||||||||||||
channelName={channelName} | ||||||||||||||||||||||||||||
visibilityType={visibilityType} | ||||||||||||||||||||||||||||
announcement={announcement} | ||||||||||||||||||||||||||||
onChangeChannelName={onChangeChannelName} | ||||||||||||||||||||||||||||
onOpenTypeSelect={onOpenVisibilityTypeSelected} | ||||||||||||||||||||||||||||
onClosedTypeSelect={onClosedVisibilityTypeSelected} | ||||||||||||||||||||||||||||
onAnnouncementSelected={onAnnouncementSelected} | ||||||||||||||||||||||||||||
/> | ||||||||||||||||||||||||||||
), | ||||||||||||||||||||||||||||
[ | ||||||||||||||||||||||||||||
channelName, | ||||||||||||||||||||||||||||
visibilityType, | ||||||||||||||||||||||||||||
announcement, | ||||||||||||||||||||||||||||
onChangeChannelName, | ||||||||||||||||||||||||||||
onOpenVisibilityTypeSelected, | ||||||||||||||||||||||||||||
onClosedVisibilityTypeSelected, | ||||||||||||||||||||||||||||
onAnnouncementSelected, | ||||||||||||||||||||||||||||
], | ||||||||||||||||||||||||||||
); | ||||||||||||||||||||||||||||
const stepperButtons = React.useMemo( | ||||||||||||||||||||||||||||
() => ({ | ||||||||||||||||||||||||||||
settings: { | ||||||||||||||||||||||||||||
nextProps: { | ||||||||||||||||||||||||||||
content: 'Next', | ||||||||||||||||||||||||||||
disabled: !channelName.trim(), | ||||||||||||||||||||||||||||
onClick: () => { | ||||||||||||||||||||||||||||
setChannelName(channelName.trim()); | ||||||||||||||||||||||||||||
atulUnsubmitted Not Done Inline ActionsWhy are we using .trim() here? It seems like it could break some equality assumptions to modify channelName like that? atul: Why are we using `.trim()` here? It seems like it //could// break some equality assumptions to… | ||||||||||||||||||||||||||||
jakubAuthorUnsubmitted Done Inline ActionsI tried to remove unnecessary whitespaces in channelName, because it could allow us to create a channel with whitespaces-only name. What equality assumptions do you mean? jakub: I tried to remove unnecessary whitespaces in `channelName`, because it could allow us to create… | ||||||||||||||||||||||||||||
setActiveStep('members'); | ||||||||||||||||||||||||||||
}, | ||||||||||||||||||||||||||||
}, | ||||||||||||||||||||||||||||
}, | ||||||||||||||||||||||||||||
members: { | ||||||||||||||||||||||||||||
prevProps: { | ||||||||||||||||||||||||||||
content: 'Back', | ||||||||||||||||||||||||||||
onClick: () => setActiveStep('settings'), | ||||||||||||||||||||||||||||
}, | ||||||||||||||||||||||||||||
nextProps: { | ||||||||||||||||||||||||||||
content: 'Create', | ||||||||||||||||||||||||||||
onClick: () => { | ||||||||||||||||||||||||||||
/// TODO: make form logic | ||||||||||||||||||||||||||||
atulUnsubmitted Done Inline ActionsWe usually don't leave TODO comments in the codebase. I think you also have an extra slash at the start of the comment (/// vs //) atul: We usually don't leave `TODO` comments in the codebase.
I think you also have an extra slash… | ||||||||||||||||||||||||||||
jakubAuthorUnsubmitted Done Inline ActionsSure, I left this comment here as an interface. This function is already implemented in the next diff. jakub: Sure, I left this comment here as an interface. This function is already implemented in the… | ||||||||||||||||||||||||||||
}, | ||||||||||||||||||||||||||||
}, | ||||||||||||||||||||||||||||
}, | ||||||||||||||||||||||||||||
}), | ||||||||||||||||||||||||||||
[channelName], | ||||||||||||||||||||||||||||
); | ||||||||||||||||||||||||||||
atulUnsubmitted Not Done Inline ActionsCan you explain what's going on here? atul: Can you explain what's going on here? | ||||||||||||||||||||||||||||
jakubAuthorUnsubmitted Done Inline ActionsThis object contains props that are provided to Stepper component (D5186) action buttons. jakub: This object contains props that are provided to `Stepper` component (D5186) action buttons.
I… | ||||||||||||||||||||||||||||
const subchannelMembers = React.useMemo( | ||||||||||||||||||||||||||||
() => ( | ||||||||||||||||||||||||||||
<SubchannelMembers | ||||||||||||||||||||||||||||
parentThreadInfo={parentThreadInfo} | ||||||||||||||||||||||||||||
selectedUsers={selectedUsers} | ||||||||||||||||||||||||||||
searchText={searchUserText} | ||||||||||||||||||||||||||||
setSearchText={setSearchUserText} | ||||||||||||||||||||||||||||
switchUser={switchUser} | ||||||||||||||||||||||||||||
/> | ||||||||||||||||||||||||||||
), | ||||||||||||||||||||||||||||
[ | ||||||||||||||||||||||||||||
selectedUsers, | ||||||||||||||||||||||||||||
switchUser, | ||||||||||||||||||||||||||||
parentThreadInfo, | ||||||||||||||||||||||||||||
searchUserText, | ||||||||||||||||||||||||||||
setSearchUserText, | ||||||||||||||||||||||||||||
], | ||||||||||||||||||||||||||||
); | ||||||||||||||||||||||||||||
steps.push( | ||||||||||||||||||||||||||||
<Stepper.Item | ||||||||||||||||||||||||||||
content={subchannelSettings} | ||||||||||||||||||||||||||||
key="settings" | ||||||||||||||||||||||||||||
nextProps={stepperButtons.settings.nextProps} | ||||||||||||||||||||||||||||
/>, | ||||||||||||||||||||||||||||
); | ||||||||||||||||||||||||||||
steps.push( | ||||||||||||||||||||||||||||
<Stepper.Item | ||||||||||||||||||||||||||||
content={subchannelMembers} | ||||||||||||||||||||||||||||
key="members" | ||||||||||||||||||||||||||||
prevProps={stepperButtons.members.prevProps} | ||||||||||||||||||||||||||||
nextProps={stepperButtons.members.nextProps} | ||||||||||||||||||||||||||||
/>, | ||||||||||||||||||||||||||||
); | ||||||||||||||||||||||||||||
tomekUnsubmitted Done Inline Actions
I think we can just create the steps with elements set from the beggining tomek: I think we can just create the `steps` with elements set from the beggining | ||||||||||||||||||||||||||||
const modalName = React.useMemo(() => { | ||||||||||||||||||||||||||||
if (activeStep !== 'settings') { | ||||||||||||||||||||||||||||
atulUnsubmitted Done Inline ActionsCan we just make this check activeStep === 'members'? atul: Can we just make this check `activeStep === 'members'`? | ||||||||||||||||||||||||||||
return `Create channel - ${trimText(channelName || '', 11)}`; | ||||||||||||||||||||||||||||
tomekUnsubmitted Not Done Inline ActionsWhy we're using exactly 11 here? tomek: Why we're using exactly `11` here? | ||||||||||||||||||||||||||||
jakubAuthorUnsubmitted Done Inline ActionsThis is tested value, that allow us to avoid resizing of the whole modal. We could also use better solution, using text-overflow: ellipsis;, but this approach might require a separate diff/task with Modal modifications. jakub: This is tested value, that allow us to avoid resizing of the whole modal. We could also use… | ||||||||||||||||||||||||||||
tomekUnsubmitted Not Done Inline ActionsYeah, it makes sense to not complicate this diff with those changes - it might be a good followup diff / task. tomek: Yeah, it makes sense to not complicate this diff with those changes - it might be a good… | ||||||||||||||||||||||||||||
} else { | ||||||||||||||||||||||||||||
return 'Create channel'; | ||||||||||||||||||||||||||||
} | ||||||||||||||||||||||||||||
}, [activeStep, channelName]); | ||||||||||||||||||||||||||||
atulUnsubmitted Done Inline ActionsThere's also no benefit to memoizing a string (unless there was some intense computation required to determine the string). The benefits of memoization come into play with objects (including arrays, functions, etc) and maintaining referential equality. Found the following resource extremely helpful, might be worth a read: https://blog.isquaredsoftware.com/2020/05/blogged-answers-a-mostly-complete-guide-to-react-rendering-behavior/ atul: There's also no benefit to memoizing a string (unless there was some intense computation… | ||||||||||||||||||||||||||||
return ( | ||||||||||||||||||||||||||||
<Modal name={modalName} onClose={props.onClose} size="fit-content"> | ||||||||||||||||||||||||||||
atulUnsubmitted Done Inline ActionsWe're pulling parentThreadInfo out of props on line 38, can we pull out onClose there as well? atul: We're pulling `parentThreadInfo` out of props on line 38, can we pull out `onClose` there as… | ||||||||||||||||||||||||||||
<ComposeSubchannelHeader parentThreadName={parentThreadName} /> | ||||||||||||||||||||||||||||
<div className={css.container}> | ||||||||||||||||||||||||||||
<div className={css.stepItem}> | ||||||||||||||||||||||||||||
<Stepper.Container | ||||||||||||||||||||||||||||
className={css.stepContainer} | ||||||||||||||||||||||||||||
activeStep={activeStep} | ||||||||||||||||||||||||||||
> | ||||||||||||||||||||||||||||
{steps} | ||||||||||||||||||||||||||||
atulUnsubmitted Done Inline ActionsCan we memoize steps? atul: Can we memoize `steps`? | ||||||||||||||||||||||||||||
</Stepper.Container> | ||||||||||||||||||||||||||||
</div> | ||||||||||||||||||||||||||||
</div> | ||||||||||||||||||||||||||||
</Modal> | ||||||||||||||||||||||||||||
); | ||||||||||||||||||||||||||||
} | ||||||||||||||||||||||||||||
export default ComposeSubchannelModal; |
We want to avoid typing things as any. Can we type this as mixed or void instead?