4
\$\begingroup\$

I have a class which is part of my page layout. It is creating header tag elements (h1 - h5). Could someone tell me how I could improve it (I know it should be improved, but don't know how)? I have problem with rewriting almost the same code in switch cases. Would rather to create some kind of <h{type}></h{type}> code, but couldn't.

import React, {Component} from 'react';
import PropTypes from 'prop-types';

export class TypographyHeader extends Component {
    createHeader() {
        let txt        = this.props.text;
        let spaceIndex = txt.indexOf(' ');
        let p2         = txt.substr(spaceIndex);

        txt =
            <span className="typography_header__distinctive_el">
                {txt.substr(0, spaceIndex)}
            </span>;

        switch(this.props.headerType) {
            case 1:
                return <h1 className="typography_header">{txt} {p2}</h1>;
            case 2:
                return <h2 className="typography_header">{txt} {p2}</h2>;
            case 3:
                return <h3 className="typography_header">{txt} {p2}</h3>;
            case 4:
                return <h4 className="typography_header">{txt} {p2}</h4>;
            case 5:
                return <h5 className="typography_header">{txt} {p2}</h5>;
        }

        return <h1 className="typography_header">{txt} {p2}</h1>;
    }

    render() {
        return (
            <div>
                {this.createHeader()}
            </div>
        );
    }
}

TypographyHeader.propTypes = {
    headerType: PropTypes.oneOf([1, 2, 3, 4, 5]).isRequired,
    text: PropTypes.string.isRequired,
};
\$\endgroup\$
4
\$\begingroup\$

Choosing the heading level at runtime:

According to the official docs, you cannot use expressions within JSX types. However, you can use capitalized variables:

const Heading = "h" + 1;
return <Heading>...</Heading>;

Further suggestions:

  • Rename TypographyHeader to TypographyHeading to avoid confusion with <header>.
  • Rename headerType to level.
  • Rename txt to text in let txt = this.props.text for consistency's sake.

  • Don't recycle the txt variable by rebinding it to a JSX <span>.

  • Consider using const variable bindings.
  • As there are 6 official heading levels, you might want to relax your propTypes constraints:

    headerType: PropTypes.oneOf([1, 2, 3, 4, 5, 6]).isRequired
    
  • Use destructuring arguments to avoid writing let txt = props.text.

  • Make the white space following the <span> element obvious by appending {" "}.
  • Handle heading texts without spaces by splitting according to this suggestion.

Proposed refactoring:

function TypographyHeading({text, level}) {

    // Split text into leading and remaining words:
    const space = text.indexOf(" ");
    const leading = text.substr(0, space);
    const remaining = text.substr(space + 1);

    // Select heading <h1> - <h5>:
    const Heading = "h" + level;

    return (
        <div>
            <Heading className="typography_header">
                <span className="typography_header__distinctive_el">
                    {leading}
                </span>
                {" "}{remaining}
            </Heading>
        </div>
    );
}
\$\endgroup\$
2
  • \$\begingroup\$ It's a very nice answer! Thanks! However, I've got a question. Why and when to useconst instead of let? Is it in times, when variable does not change and for smaller memory usage? \$\endgroup\$ May 7 '17 at 10:57
  • \$\begingroup\$ @KrzysztofTrzos Not rebinding ('recycling') a variable helps understanding and debugging of your code. Using const enforces that rule. Some enthusiasts recommend to use const wherever applicable. Which however can lead to a strange (IMHO) mix of let and const depending on whether you deal with primitive types or objects. \$\endgroup\$
    – le_m
    May 7 '17 at 14:23

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.