[WK5-Th/F] Resolving comments on PR

1. Waking up

I had a dinner with someone I did discipleship last year with. I got home at like 11pm, and helped dad transfer his stuffs from old phones to a new one. So I didn't really get a chance to write any stuff yesterday, which is sort of annoying because I feel obliged to write stuff for yesterday. But I guess good thing is that we have summer fridays where we finish our work 4 hours earlier than usual (so it would be 1pm for me). That means less things to write for today.

 

What's more, I've just been working on resolving comments on my PR for yesterday and today, so Thursday post was going to be similar to Friday anyways.

 

Anyways, back onto the usual stuff. Today, I woke up at 8:30, despite the fact that I slept at around 2. I wasn't even too tired. I hear that it's because I woke up at the right time (like, when I'm not in a deep sleep?). So maybe my ideal sleeping time is like 6 hours and 30 minutes? Or it could just be a fluke and I might feel way tired for tomorrow. For the whole lifetime I thought 10 hours is my required sleeping time (although since yr 9 I've rarely been getting that much sleep). Maybe I'm growing up, idk.

 2. Lots, lots of comments on PR!

Well, not 'lots lots' but still it was the most I've gotten so far. And for those new comers who don't know the meaning of 'comments on PR', PR (pull request) is just the changes I make that have to be reviewed and merged into master branch (main, stable production codebase). Getting comments mean that there are things to fix. In my case, I need 2 approvals and I am still fixing stuffs 2 days after I raised the pull request. So, here's me writing tips for future self to reduce the chance of getting comments and increase the chance of getting PR approved!

2.1 Add proptype

Proptype (prop-type) is something that I thought was a random library, but it's a library supported by React, and was originally part of React namespace. It's similar to interface in typescript, and it defines what props of what type are required and what the default values would be for some / all of those properties.

 

Whenever you are adding a prop to a component, you should consider this.

import PropTypes from 'prop-types'
const SomeComponent = (prop)=>{
	const {a,b,c} = prop
	return (...)
}

SomeComponent.propTypes = {
	a: PropTypes.oneOfType([PropTypes.string,PropTypes.number]),
    b: PropTypes.bool,
    c: PropTypes.string.required
}

SomeComponent.defaultProps = {
	a: "Hey",
    b: true
}

2.2 Using null for className

Use null for className prop if you don't assign a class.

 

2.3 Regarding inevitable magic numbers - add comments

 

2.4 When a prop is simply passed in without default value

const Hello = (props)=>{
    const {a,b,c, ...otherProps} = props
    return (<SomeComponent a={a} b={b} c={c} {...otherProps} />)
}

Hello.PropTypes = {
    a: PropTypes.number
    b: PropTypes.number
    c: PropTypes.number
}

Hello.DefaultProps = {
    a: 4,
    b: 2
}

Have a look at property c. 'c' doesn't have a default prop, and is simply being passed into SomeComponent. In other words, there's no reason for c to be different from any of the props in 'otherProps'. So it's better to have something like this:

const Hello = (props)=>{
    const {a,b,c, ...otherProps} = props
    return (<SomeComponent a={a} b={b} {...otherProps} />)
}

Hello.PropTypes = {
    a: PropTypes.number
    b: PropTypes.number
}

Hello.DefaultProps = {
    a: 4,
    b: 2
}

It's cleaner, and c doesn't distract you anymore. Even if client code uses it as `<Hello c={2} />`, there's no reason for it to be present in `const {a,b...=props`, because of the fact that 1) it doesn't have a default value (so no need to separate it out from 'other props'), 2) it is not used in the code other than where it is simply passed into a component.

 

In practice, I encountered it while working with a component (BaseTypography) that wraps around Material UI component (Typography). Since we don't need a default value to variant, and since it is merely passed into Typography, we can simply have it part of object spread of otherProps.

 

2.5 Assign default value to non-required prop (prop-type) 

This one's for code style, and is detected by lint. We assign default prop to nonrequired prop, even if it be null or undefined. Why do we give undefined to nonrequired prop? Wouldn't it just be undefined as it is? Yes, but it's a good practice to do so, and is part of the lint. Can't remember which rule but you can read over that if you want.

const Hello = (props)=>{
    const {a,b,c, ...otherProps} = props
    return (<SomeComponent a={a} b={b} {...otherProps} />)
}

Hello.PropTypes = {
    a: PropTypes.number.required
    b: PropTypes.number
}

Hello.DefaultProps = {
    b: 2
}

2.6 Adding story for a new prop to Storybook

Whenever a new prop is made, it's a good idea to add it as a story so it can be observed in isolation. So in my case, since ellipsis was added, I added the story for ellipsis.