Skip to content

Conversation

ssi02014
Copy link
Contributor

@ssi02014 ssi02014 commented Aug 2, 2023

Hello 👋, @wojtekmaj
Thanks for sharing this great library.
I fixed a typo in the component name.

export const isViews = PropTypes.arrayOf(PropTypes.oneOf(allViews));

export const isView: Requireable<View> = function isView(props, propName, componentName) {
export const isView: Requireable<View> = function (props, propName, componentName) {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

We don't think you need a function name for that code.

Copy link
Owner

Choose a reason for hiding this comment

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

This makes sure we're getting the right error stack of something throws an error.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I missed it, thanks

minDate?: Date;
setMaxDate: (date?: Date) => void;
setMinDate: (date?: Date) => void;
};
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think it would be cleaner to use Optional Property.

Copy link
Owner

Choose a reason for hiding this comment

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

There's a significant difference between "this property must be passed, but may be undefined somewhere down the line" and "you can forget about passing that property and it will be alright". It's purposeful.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@wojtekmaj Wow, that's right, I get it.

@wojtekmaj
Copy link
Owner

Hi!

Most of your changes, removing React.FC are causing duplicated types to be produced, resulting in errors in some IDEs. This has been fixed in 70bd390 and I have no plans of rolling it back unless a better solution is proposed.

The typo is legit though. :D

@ssi02014
Copy link
Contributor Author

ssi02014 commented Aug 2, 2023

@wojtekmaj
Thanks for the review. 🙏
I'll revert the part about React.FC.

@ssi02014
Copy link
Contributor Author

ssi02014 commented Aug 2, 2023

@wojtekmaj
Thank you for informing me about 70bd390.
I have modified the code again.

@ssi02014 ssi02014 requested a review from wojtekmaj August 2, 2023 16:56
@wojtekmaj wojtekmaj changed the title refac(react-calendar): Refactoring react-calendar Fix typo in component name Aug 2, 2023
@wojtekmaj wojtekmaj enabled auto-merge (squash) August 2, 2023 16:58
@wojtekmaj wojtekmaj merged commit 3cf9999 into wojtekmaj:main Aug 2, 2023
@ssi02014 ssi02014 deleted the refac/react-calendar branch August 2, 2023 16:59
@wojtekmaj
Copy link
Owner

Thanks! 👍

@ssi02014
Copy link
Contributor Author

ssi02014 commented Aug 2, 2023

@wojtekmaj
Thank you for the detailed review.
I am pleased to make a small contribution to the react-calendar repository 🎉

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