We're going to have a couple of modals that have search functionality, so it makes sense to have a component for that.
We usually prefer using children components when introducing containers like this, but in this case that approach has a couple of disadvantages: we would need to pass searchText to children as a prop and that would require using React.cloneElement. Also, defining child component props would be complicated to allow supporting all the components that have searchText props. None of these are too serious, but it looks like using children function is more readable and easier approach. If you disagree, it can easily be modified.
Depends on D3385
Details
Open members modal and check if filtering works correctly.
Diff Detail
- Repository
- rCOMM Comm
- Branch
- merge
- Lint
No Lint Coverage - Unit
No Test Coverage
Event Timeline
web/modals/threads/members/members-modal.css | ||
---|---|---|
2 ↗ | (On Diff #10604) | This property was duplicated |
Shouldn't we keep "one component per one file" approach? What is the convention in our project - when can/should we put some components together in one file?
web/modals/search-modal.react.js | ||
---|---|---|
18 ↗ | (On Diff #10604) | nit: could we move the prop destructure to line 17 under the function definition? |
19 ↗ | (On Diff #10604) | This might be a personal preference but I think children as a render prop is hard to grok. I know they're still very much a thing. Also reading the description above not sure what a better solution is. |
22 ↗ | (On Diff #10604) | nit: add emty line between line 22 and return. |
Shouldn't we keep "one component per one file" approach? What is the convention in our project - when can/should we put some components together in one file?
I think it's okay to have a second component in a file if it is only used by the primary component in that file.
web/modals/search-modal.react.js | ||
---|---|---|
19 ↗ | (On Diff #10604) | I agree with @benschac about children as a render prop. I know it's a pattern, but I find it confusing. Why is it necessary here? Why not just pass a React.Node? |
Requesting review without updating the code because we need to clarify the approach in inline comment first
web/modals/search-modal.react.js | ||
---|---|---|
19 ↗ | (On Diff #10604) | As I said in the summary:
So basically, we need to render children with an additional prop searchText that is provided by this component. To do that for React.Node we would need to clone that element to add a new prop. I think that using cloneElement is even harder to grok than render prop, because render prop is just a function and is clearly described by props, while we see cloneElement really rarely in the code and that could confuse developers even more. Additionally, in members modal we would need to give SearchModal a default searchText. And finally, we can't use just React.Node because we would need a single child that has a prop searchText - typing that is more complicated. For me, these three make render prop a better solution. If you think otherwise, I can change the code - that would be quick as I used React.Node approach as the first iteration and have it in my stash. @ashoat |
As I said in the summary:
we would need to pass searchText to children as a prop and that would require using React.cloneElement
Oops, sorry – I totally missed that!
And finally, we can't use just React.Node because we would need a single child that has a prop searchText - typing that is more complicated.
This is an interesting problem, so I spent 5 minutes trying to get it to work. I think this is what you want. Not sure React.cloneElement is worth it, though...
For me, these three make render prop a better solution. If you think otherwise, I can change the code
One possible alternative would be to use React.Context to pass the searchText to all descendants, but I'm not sure it's worth it.
Ultimately defer to you – I understand why you're using a render prop here.
And finally, we can't use just React.Node because we would need a single child that has a prop searchText - typing that is more complicated.
This is an interesting problem, so I spent 5 minutes trying to get it to work. I think this is what you want. Not sure React.cloneElement is worth it, though...
Interesting... looks easier than what I initially implemented
export type SearchModalChildProps = { +searchText: string, }; type Props<C> = { +name: string, +searchPlaceholder: string, +onClose: () => void, +children: React.Element<React.ComponentType<C>>, }; export function SearchModal<C: SearchModalChildProps>( props: Props<C>, ): React.Node { type ContentProps = { ...SearchModalChildProps, +threadID: string, }; function ThreadMembersModalContent(props: ContentProps): React.Node {
Regarding cloneElement - I also don't think it is worth it
For me, these three make render prop a better solution. If you think otherwise, I can change the code
One possible alternative would be to use React.Context to pass the searchText to all descendants, but I'm not sure it's worth it.
It would make the solution a lot more complicated, so I don't think it will be better that the current solution where only the render prop is an issue
Ultimately defer to you – I understand why you're using a render prop here.
Ok, so in that case I'll keep the render prop
web/modals/search-modal.react.js | ||
---|---|---|
18 ↗ | (On Diff #10604) | We're using children in the next line which comes from props and I don't think that having to destructure props twice is beneficial here. |