Convert ParametersList to TS #42822
Convert ParametersList to TS #42822
Conversation
|
oisincoveney
force-pushed
the
refactor-dashboard-controls
branch
from
8dcfa8c
to
2abc506
Compare
oisincoveney
force-pushed
the
convert-parameters-list-to-ts
branch
from
360818f
to
3b67391
Compare
| @@ -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.
I suppose the value here can be of multiple types?
There was a problem hiding this comment.
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.
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.
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.
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.
This makes it accept both undefined and null. You can also do hiddenParameterSlugs: string | null = null if you only need null.
oisincoveney
force-pushed
the
convert-parameters-list-to-ts
branch
from
331d42d
to
b02b40c
Compare
oisincoveney
force-pushed
the
convert-parameters-list-to-ts
branch
from
b02b40c
to
30d0019
Compare
|
Please also address #42777 (comment) in scope of this PR. |
oisincoveney
force-pushed
the
convert-parameters-list-to-ts
branch
from
08c7fe2
to
f8dc4a0
Compare
oisincoveney
force-pushed
the
convert-parameters-list-to-ts
branch
from
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
ParametersListandSyncedParametersListto TS. It also unifies the props so there's less redundancy in type declarations