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

Convert common helpers to Typescript #2541

Merged

Conversation

daniellesniak
Copy link

See #3533

Copy link
Member

@askvortsov1 askvortsov1 left a comment

Choose a reason for hiding this comment

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

Looking good, just a few nitpicks

* @param {User} user
* @param {Object} attrs Attributes to apply to the avatar element
* @return {Object}
* @param attrs Attributes to apply to the avatar element
Copy link
Member

Choose a reason for hiding this comment

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

I wouldn't want to have one param documented but the other not

Copy link
Member

Choose a reason for hiding this comment

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

The other is still documented in the typehinting. Attrs only remains for the description in the JSDoc. I don't know if we want to do that or keep tham all, howeverr 🤷

Copy link
Author

Choose a reason for hiding this comment

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

I think we should leave all parameters in JSDoc if there's any with description and get rid of all parameters if none of them have any description. By the way this should resolve PHPStorm's warning "Parameter is not described in JSDoc".

js/src/common/helpers/avatar.tsx Outdated Show resolved Hide resolved
Copy link
Member

@askvortsov1 askvortsov1 left a comment

Choose a reason for hiding this comment

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

Thank you!

Copy link
Member

@dsevillamartin dsevillamartin left a comment

Choose a reason for hiding this comment

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

👍

@askvortsov1 askvortsov1 merged commit a65e1de into flarum:master Mar 3, 2021
@daniellesniak daniellesniak deleted the dl/typescript-common-helpers branch March 5, 2021 16:30
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.

3 participants