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

[svg] / [design-system] - Update explicit types, and added eslint config to svg #82

Closed

Conversation

nebula-aac
Copy link
Contributor

Notes for Reviewers

This PR fixes #

Signed commits

  • Yes, I signed my commits.

@aabidsofi19
Copy link
Contributor

why did we change it to JSX.Element (curious)

@nebula-aac
Copy link
Contributor Author

The updates comes from eslint rule

Here is an example of error when you run eslint.

/workspaces/sistent/packages/components/src/base/Accordion/accordionsummary.tsx
  4:8  error  Missing return type on function  @typescript-eslint/explicit-function-return-type

To satisfy the rule, we can explicitly label JSX.Element which means each of the component is returning an JSX.Element

We can either turn this rule off to make it less explicit, and allow some flexibility in using the components, or leave this rule on, and make sure that each of the component is returning an JSX.Element. This will allow Typescript to perform better type checking and also allow everyone else who is contributing to know this this is a particular type.

I'm open to either way.

@nebula-aac
Copy link
Contributor Author

@aabidsofi19 Can you review again and see if this can be merged in.

I have more that I want to add, and don't want to create multiple PRs and rebasing the PRs if I can avoid it.

@aabidsofi19
Copy link
Contributor

@nebula-aac sorry , i forgot to review some of the prs ..

@aabidsofi19
Copy link
Contributor

@nebula-aac i think the return type should be React.FC rather than JSX.Element , because it is a react component as it takes props

@leecalcote
Copy link
Member

Merge conflicts cropped up...

@nebula-aac
Copy link
Contributor Author

I'm creating a different PR, closing this one.

@nebula-aac nebula-aac closed this Sep 10, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Development

Successfully merging this pull request may close these issues.

3 participants