Intro
I refact a dropdown composition component. Changing it to object.
We had a dropdown in the following structure:
<Dropdown>
<Dropdown.Toggle color='primary' />
<Dropdown.Menu>
<Dropdown.Item onClick={() => {}}>
MyLabel
</Dropdown.Item>
<Dropdown.Item onClick={() => {}}>
AnotherLabel
</Dropdown.Item>
</Dropdown.Menu>
</Dropdown>
Problem
When we need to hide some elements, we need to apply conditional rendering and with that, we have a mixture of javascript with jsx syntax that makes the code difficult to read and difficult to maintain. For example:
<Page>
<Subheader helpText="Choose a Publication action">
<Dropdown>
<Dropdown.Toggle color='primary' />
<Dropdown.Menu>
{allowEdit ?
(
<Dropdown.Item onClick={handleTransfer}>
Transfer
</Dropdown.Item>
<Dropdown.Item onClick={handleAbandon}>
Abandon
</Dropdown.Item>
) : (
{hasPermission('Pages.Publications.Publish') ? (
<Dropdown.Item onClick={handlePublish}>
Publish
</Dropdown.Item>
) : (
<Dropdown.Item onClick={handleTransfer}>
Transfer
</Dropdown.Item>
)}
)
)}
</Dropdown.Menu>
</Dropdown>
</Subheader>
<Content>
...
</Content>
<Page>
Realize that there is a ternary inside the other, and also, there is a code duplication for the 'Transfer' item. This is terrible to read.
If allowEdit is TRUE, it should render the 'Transfer' and 'Abandon' items:
If allowEdit is FALSE and user has permission to Publish, it should render only 'Publish' item.
Or 'Transfer' for fallback.
Proposal
Create a new component (DropdownButton) to support options that receive list items instead of children. Like this:
<DropdownButton color='primary' options={[
{ label: '', onClick: () => {}, visible: true }
{ label: '', onClick: () => {}, visible: true }
]}
/>
The visible property should control when showing the dropdown item.
Benefits
- The code is easier to read
- Less code repetition
- We can import the object from elsewhere
And applying our conditional rendering...
<Page>
<Subheader helpText="Choose a Publication action">
<DropdownButton
color='primary'
options={[
{
label: 'Transfer',
onClick: handleTransfer,
visible: allowEdit || !hasPermission('Pages.Publications.Publish')
},
{
label: 'Abandon',
onClick: handleAbandon,
visible: allowEdit
},
{
label: 'Publish',
onClick: handlePublish,
visible: !allowEdit && hasPermission('Pages.Publications.Publish')
},
]}
/>
</Subheader>
<Content>
...
</Content>
<Page>
Conclusion
We are used to writing more declarative code, always thinking about generic code, but sometimes it makes our code difficult to read and maintain.
Props as object items are widely used and we can use them when we want to better control our components. In this refactoring, we've assigned responsibility for the conditional rendering requirements to the visible prop.
Top comments (4)
Composition is almost always better than not using composition. Its kind of a bad practice in React to create components that take in huge config objects through props, because it indicates that component has way too many responsibilities. The usage might look clean, but the implementation won't. If you need to conditionally render something, its trivial to add a wrapping component:
Just keep in mind that
YourComponentis mounted regardless of the outcome of the condition, but it won't appear in the DOM if the condition is false.Yes, I agree that very complex props can take a lot of responsibility, but in this case it's a simple dropdown with label + handle + visible. I prefer :D
Thanks for the comment.
Yes but at some point in the future you want to add some other functionality, so you adjust the config and add even more complexity to your single master component. With composition, if you want to add some special menu item, you don't have to change any code, you just extend the existing menu item by wrapping it in your new special functionality menu item component. Having to change the responsibilities of an existing component, when a client asks for new functionality, is a strong indicator that your code is too rigid.
Composition in React is a very valuable skill to learn.
Nice