Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Transition project to typescript #6

Open
wants to merge 10 commits into
base: master
Choose a base branch
from
Open

Transition project to typescript #6

wants to merge 10 commits into from

Conversation

maker0101
Copy link
Owner

No description provided.

@maker0101 maker0101 requested a review from snqb April 19, 2022 10:39
@maker0101 maker0101 self-assigned this Apr 19, 2022
package.json Outdated Show resolved Hide resolved
import classNames from 'classnames';

const Button = ({
interface ButtonProps {
children: string;
Copy link
Collaborator

@snqb snqb Apr 20, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

  1. FC already has types for children, so no need to put it here.
  2. children are ReactNode AFAIK, not string. (ReactNode includes string)

Copy link
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

  1. Good to know, thanks. I read afterwards that it's a good idea to make it explicit, when a component is accepting children (as some components don't). Thus it might be good to leave it. Do you agree?

  2. In this Button case I actually only want text as children. Is string ok then because it is more specific?

Comment on lines 25 to 26
onClick,
}) => {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

a lot of times it's a good practice in cases like this when you're extending an HTML button, to extend its props like
interface ButtonProps extends HTMLButtonElement

and then use the ...props notation to pass some native button props to the <button> inside

Copy link
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Awesome! Thanks so much Esen. This actually answered a question I had lurking in my mind for a while now about how to easily extend all HTML attributes to a component without writing tons of code.

I've chosen ComponentProps<"button"> over HTMLAttributes<HTMLButtonElement>after reading this article: https://dev.indooroutdoor.io/how-to-extend-any-html-element-with-react-and-typescript

Would you agree it's an even better choice?

Comment on lines +5 to +7
interface ContentCenterProps {
children: ReactNode;
}
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

children are in FC already

Copy link
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I have to admit FC with Typescript confuses me :).

First, I stumbled upon sooo many ressources discouraging the use of FC, e.g. https://fettblog.eu/typescript-react-why-i-dont-use-react-fc/.
So FC automatically adds children type to all React components? That's probably bas I want to be explicit about it, correct? What's your take on it?

Then, based on you comment I tried FC without children, but get Typescript errors. Here is what I tried:
const Content: FC = ({ children }) => { return <div className='Content'>{children}</div>; };

Error message: 'Property children does not exist on type {}'. What am I doing wrong?

src/utilities/create-coin.ts Outdated Show resolved Hide resolved
src/utilities/create-coin.ts Outdated Show resolved Hide resolved
Questions.md Outdated Show resolved Hide resolved
Questions.md Outdated Show resolved Hide resolved
Questions.md Outdated Show resolved Hide resolved
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants