-
Notifications
You must be signed in to change notification settings - Fork 163
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
[#1936] Migrate c-summary-charts.vue to TypeScript #1976
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@vvidday Just a minor point I want to point out. The rest LGTM!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I have reviewed the code and overall, the TypeScript migration looks great!
However, I do have a comment about the prop types.
I noticed we are using generics to define the types under props
in c-summary-charts
as Array<string>
for example, but in c-summary
, we are making use of PropType
so the equivalent here would be Array as PropType<string[]>
.
Similarly, most of the attributes under data()
are defined as [] as string[]
but we could make use of generics here as well as [] as Array<string>
.
Do you think we can standardize how we define our types using a lint rule?
Co-authored-by: Charisma Kausar <[email protected]>
Great point about generics vs There does seem to be a lint rule for this, we can either choose generics or the |
@vvidday Personally I think the |
I think I would prefer the |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM. Not sure how to work with the cypress warning regarding the unexpected console statement. I do think it is useful in some cases. Maybe a separate issue could be raise for this for new developers to work on?
The following links are for previewing this pull request:
|
Part of #1936
Proposed commit message
Other information
window.getAuthorDisplayName
type definitionThis PR changes the type for the
authorRepos
parameter inwindow.getAuthorDisplayName
, fromRepo[]
toUser[]
. This was a mistake I made in the initial type definition (as bothRepo
andUser
have thedisplayName
attribute).