-
Notifications
You must be signed in to change notification settings - Fork 4.9k
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 ParametersList to TS #42822
Convert ParametersList to TS #42822
Conversation
|
8dcfa8c
to
2abc506
Compare
360818f
to
3b67391
Compare
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.
Looks good to me so far! Just a couple of nits and questions.
@@ -66,8 +72,7 @@ function ParametersList({ | |||
setEditingParameter={setEditingParameter} | |||
setValue={ | |||
setParameterValue && | |||
(value => | |||
setParameterValue(valuePopulatedParameter.id, value, dashboard?.id)) | |||
((value: any) => setParameterValue(valuePopulatedParameter.id, value)) |
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 suppose the value
here can be of multiple types?
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.
yep, I'll actually change this to unknown
since the setParameterValue
action expects:
(parameterId: ParameterId, value: unknown)
dashboard?: Dashboard | null; | ||
editingParameter?: Parameter | null; |
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.
This makes these parameters accept both undefined
and null
. If we know that it only accepts null
and not undefined
we can also update the type here to be more strict (e.g. dashboard: Dashboard | null
. Otherwise ok to leave as-is.
} & Pick<DashboardFullscreenControls, "isFullscreen"> & | ||
Pick<DashboardThemeControls, "isNightMode"> & | ||
Pick<DashboardHideParametersControls, "hideParameters"> |
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.
Since isFullscreen
and isNightMode
is a plain boolean, I think we can inline them.
However, I suppose the intent here is to be aware of a breaking change when isFullscreen
disappears from the DashboardFullscreenControls
, so we know that this type has to be updated? i.e. to communicate that the prop is passed down from those components. If so, we can leave this as-is.
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.
Exactly - since we're prop-drilling these all the way down to ParametersList, I think it would make more sense to ensure that the type remains the same all the way down the chain
@@ -32,7 +32,7 @@ export function buildHiddenParametersSlugSet( | |||
|
|||
export function getVisibleParameters( | |||
parameters: UiParameter[], | |||
hiddenParameterSlugs?: string, | |||
hiddenParameterSlugs?: string | null, |
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.
This makes it accept both undefined
and null
. You can also do hiddenParameterSlugs: string | null = null
if you only need null
.
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.
Much better 😄
331d42d
to
b02b40c
Compare
b02b40c
to
30d0019
Compare
Please also address #42777 (comment) in scope of this PR. |
08c7fe2
to
f8dc4a0
Compare
8860162
to
0795a99
Compare
I'll do it in #43221 so we can separate the PRs and isolate the logic for a better review |
@oisincoveney Did you forget to add a milestone to the issue for this PR? When and where should I add a milestone? |
Converts
ParametersList
andSyncedParametersList
to TS. It also unifies the props so there's less redundancy in type declarations