-1

In our codebase, I keep coming across this pattern of invoking renderXelement() inside the render method. I have heard that it is not a very common pattern for React. Also, I was wondering whether is a thing to avoid at all costs, in a sense that what kind of downsides it has (if any).

See renderCollapsedLinks(). I removed some of the excessive code to make the question more clear.

Example:

export function NavigationDropdownLinksOverlay({
    activeDropdownLink,
    onClickBack,
}: NavigationDropdownLinkOverlay) {
    const { classes } = useStyles();

    const renderCollapsedLinks = (groupLink: DropdownGroupLinks) => {
        return groupLink.links.map((link, index) => (
            <Box mb={index !== groupLink.links.length - 1 ? 'sm' : 0}>
                <Link href={link.link} className={classes.anchorLink}>
                    {link.label}
                </Link>
            </Box>
        ));
    };

    return (
        <MobileNavigationOverlay show={!!activeDropdownLink}>
            <MobileMenuBanner title={activeDropdownLink?.label || ''} />
            <BackButton onClick={onClickBack} />
            <Grid gutter={0} className={classes.linkGridFullWidth}>
              {renderCollapsedLinks()} 
            </Grid>
        </MobileNavigationOverlay>
    );
} 
Berin Aptula
  • 234
  • 1
  • 12
  • It is probably fine, however you could extract it to a component `` or skip the function part and render where the function is called.. – TryingToImprove Jun 26 '23 at 07:27
  • I am on the side of just creating new components, but the codebase is large, and not sure if it would be worth the effort. I would have to make some strong points (other than it being a weird pattern :) ). – Berin Aptula Jun 26 '23 at 07:29
  • This is fine :) – Konrad Jun 26 '23 at 07:33
  • Take a look https://stackoverflow.com/questions/67185279/react-jsx-in-a-variable-vs-a-function-vs-a-separate-component – Lin Du Jun 26 '23 at 07:35
  • You can also just use a variable instead of a function. `const collapsedLinks = groupedLinks.map(...)`. If you need it only once like here a variable is probably faster – mousetail Jun 26 '23 at 07:41
  • There is nothing wrong with splitting up a big chunk of jsx markup into smaller parts to help improve readability. I sometimes do this myself. But it is a soft indicator that maybe your component does too much and is getting too big and unwieldy. I tend to either inline the `items.map(...)` calls or break some part of the elements out to define their own component. – Martin Jun 26 '23 at 07:59
  • Hmm, I understand. So is not as bad as I thought :p – Berin Aptula Jun 26 '23 at 08:15
  • @BerinAptula `{renderCollapsedLinks()}` the function expects an argument when you call it. If you add a second argument `classes` to `renderCollapsedLinks` you can move the function completely out of the component (`NavigationDropdownLinksOverlay`) since it has no dependencies on the surrounding scopes anymore. – Thomas Jun 26 '23 at 08:15

1 Answers1

1

Of course you can use functions to compute a value. Even in React, even if this value is JSX.

You can even go a step further and move this function out of the component. This makes imo. the component more readable and has the advantage that this function is no longer created on every render:

export function NavigationDropdownLinksOverlay({
    activeDropdownLink,
    onClickBack,
}: NavigationDropdownLinkOverlay) {
    const { classes } = useStyles();

    return (
        <MobileNavigationOverlay show={!!activeDropdownLink}>
            <MobileMenuBanner title={activeDropdownLink?.label || ''} />
            <BackButton onClick={onClickBack} />
            <Grid gutter={0} className={classes.linkGridFullWidth}>
                {renderCollapsedLinks(groupLink.links, classes)}
            </Grid>
        </MobileNavigationOverlay>
    );

}

// renderCollapsedLinks is now a pure function and relies only on the function arguments to produce the returned value.
const renderCollapsedLinks = (links, classes) => {
    const last = links.length-1;

    return links.map((link, index) => (
        // you should add a `key` to `<Box />`
        <Box mb={index !== last ? 'sm' : 0}>
            <Link href={link.link} className={classes.anchorLink}>
                {link.label}
            </Link>
        </Box>
    ));
};
Thomas
  • 11,958
  • 1
  • 14
  • 23