Type cleanups #4042
Type cleanups #4042
Conversation
| $: comparisonPercChange = | ||
| comparisonValue && value !== undefined && value !== null | ||
| ? (value - comparisonValue) / comparisonValue | ||
| : undefined; |
There was a problem hiding this comment.
calculate comparisonPercChange here, no need to pass it down from parent
| expandedMeasureName | ||
| ? (measure) => measure.name === expandedMeasureName | ||
| : (_, i) => $showHideMeasures.selectedItems[i], | ||
| ); |
There was a problem hiding this comment.
consolidating into one declaration allows automatic type inference
| @@ -34,7 +35,7 @@ A simple composable container for SVG-based data graphics. | |||
| export let shareYScale = true; | |||
|
|
|||
| export let mouseoverValue: DomainCoordinates | undefined = undefined; | |||
| export let hovered = undefined; | |||
| export let hovered = false; | |||
There was a problem hiding this comment.
@djbarnwal -- I believe that hover state should always default to false unless explicitly set (here and in other files), rather than using undefined. Please LMK if that will cause any problems
There was a problem hiding this comment.
Sounds about right. Shouldn't cause any issue.
| @@ -1,7 +1,7 @@ | |||
| export interface Point { | |||
| x: number; | |||
| y: number; | |||
| value: string; | |||
| value?: string; | |||
There was a problem hiding this comment.
this is already treated as optional, there are guards against it being undefined in web-common/src/components/data-graphic/marks/MultiMetricMouseoverLabel.svelte line 204
| xType: string; // FIXME: we should have an enum here | ||
| yType: string; // FIXME: we should have an enum here | ||
| xType: ScaleType; | ||
| yType: ScaleType; |
There was a problem hiding this comment.
fixed this ancient FIXME
| export let highlightedCol: number; | ||
| export let scrubPos: { start: number; end: number }; | ||
| export let highlightedCol: number | undefined; | ||
| export let scrubPos: { start?: number; end?: number }; |
There was a problem hiding this comment.
already treated as optional, see check below
| // for at least 19 months, so hopefully it's safe enough. | ||
| // In any case, this cascading context store approach is | ||
| // ridiculously over engineered and needs to be rethought | ||
| } as SimpleDataGraphicConfigurationArguments; |
There was a problem hiding this comment.
Just to add, this is most probably not a safe cast. I remember seeing missing properties at couple of places some time back. We should get back to this when we have more bandwidth to handle our tech debt
There was a problem hiding this comment.
Thanks for flagging this @djbarnwal !!! -- since you know this is unsafe for sure, I'm going to remove this cast before merging
| @@ -34,7 +35,7 @@ A simple composable container for SVG-based data graphics. | |||
| export let shareYScale = true; | |||
|
|
|||
| export let mouseoverValue: DomainCoordinates | undefined = undefined; | |||
| export let hovered = undefined; | |||
| export let hovered = false; | |||
There was a problem hiding this comment.
Sounds about right. Shouldn't cause any issue.
* Type cleanups * replace "Etc/UTC" -> "UTC" * correct types on TDDTable * use enum for scale type * finish replacing scaleType enum * fix domain coordinate types * finish setting default for selectedTimezone * add default for selectedTimeZone * fix enum import problem * make "value" optional * update svelte-check exclusions * cleanups * remove unsafe cast
part of #3424, hopefully helps with #3989, but it's not clear what exactly is causing that error.
Note that per conversation with @djbarnwal we're updating
MetricsExplorerEntity.selectedTimezoneto(a) be required, not optional)
(b) default to "UTC" if not specified,
(c) use the value "UTC" in preference to "Etc/UTC" in all cases