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

Add return type to home indicator component #47

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

isilher
Copy link

@isilher isilher commented Jun 8, 2022

Thank you for making this library and for adding TypeScript to it! When updating to v0.2.9 I found that although the properties are now properly typed, my TS configuration still gave me a warning:

error TS7011: Function expression, which lacks return-type annotation, implicitly has an 'any' return type.

By only typing the props, the HomeIndicator component was missing a clear return type. This proposed change instead types the functional component, setting the return type to be a React Functional Component that will accept the properties defined in Props.

Cheers!

By only typing the props, the HomeIndicator component was missing a clear return type. This change instead types the functional component, setting the return type to be a React Functional Component that will accept the properties defined in Props.
@codecov
Copy link

codecov bot commented Jun 8, 2022

Codecov Report

Merging #47 (1173601) into master (ff3879b) will not change coverage.
The diff coverage is 100.00%.

@@            Coverage Diff            @@
##            master       #47   +/-   ##
=========================================
  Coverage   100.00%   100.00%           
=========================================
  Files            1         1           
  Lines           25        25           
  Branches         4         4           
=========================================
  Hits            25        25           
Impacted Files Coverage Δ
index.tsx 100.00% <100.00%> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update ff3879b...1173601. Read the comment docs.

@@ -6,7 +6,7 @@ const isIos = Platform.OS === "ios";

type Props = { autoHidden: boolean };

export const HomeIndicator = (props: Props) => {
export const HomeIndicator: React.FC<Props> = (props) => {
Copy link
Member

Choose a reason for hiding this comment

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

the problem with React.FC is that it implicitly adds children to the props.
export const HomeIndicator = (props: Props): ReactElement | null => { could work, but this is a bit unusual

Copy link
Author

Choose a reason for hiding this comment

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

That is a good point and a reason to move away from using React.FC in general. Your alternative seems reasonable to me, and for TypeScript support helps us more than any, why would it be unusual in your opinion?

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