[D&D] Persists index field on agg type change if possible #2227
Conversation
Codecov Report
@@ Coverage Diff @@
## main #2227 +/- ##
==========================================
- Coverage 66.78% 66.77% -0.01%
==========================================
Files 3133 3133
Lines 60079 60086 +7
Branches 9153 9155 +2
==========================================
Hits 40121 40121
- Misses 17767 17774 +7
Partials 2191 2191
Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here. |
There was a problem hiding this comment.
Thanks, I'm excited to have this fix! I didn't pull this down to validate that it works, but I'll trust that it does.
My only concern is that it seems like we'd prefer to solve some of the type issues at the root rather than casting, but I understand the pragmatic approach for now.
| @@ -4,15 +4,15 @@ | |||
| */ | |||
|
|
|||
| import React, { useCallback, useMemo, useState } from 'react'; | |||
| import { cloneDeep } from 'lodash'; | |||
| import { cloneDeep, get } from 'lodash'; | |||
There was a problem hiding this comment.
I blame typescript. I gave into this after fighting with typescript to get the same behavior for this:
const indexField = availableFields.find(
({ name }) => name === get(draftAgg, 'params.field')
);| @@ -109,6 +109,20 @@ export function SecondaryPanel() { | |||
| }} | |||
| onAggTypeChange={function (aggId: string, aggType: IAggType): void { | |||
| aggConfig.type = aggType; | |||
|
|
|||
| // Persist field if the new agg type supports the existing field | |||
| const fieldParam = (aggType.params as AggParam[]).find(({ type }) => type === 'field'); | |||
There was a problem hiding this comment.
do we need the cast because the agg service doesn't give us suitable types?
There was a problem hiding this comment.
I'll be honest, that service has some seriously confusing typing going on that seems to confuse typescript a lot. Here the issue was that the line below where i try to getAvailableFields has a type error that only goes away when I cast these types. And these are valid types for those objects. Its just the unions and the parent child relationships between these types that is confusing the typescript compiler
|
I agree. The only reason i chose to do it this way was because I didnt want to modify the behavior of the underlying component because it was shared with the Visualize app too. Since we'll likely change this entire component to be more user friendly, I wanted to limit the blast radius for this fix. |
Signed-off-by: Ashwin Pc <[email protected]>
joshuarrrr
force-pushed
the
feat/persist-field
branch
from
a6be08e
to
38e7c2c
Compare
Signed-off-by: Ashwin Pc <[email protected]> (cherry picked from commit 4a06f5a)
Signed-off-by: Ashwin Pc <[email protected]> (cherry picked from commit 4a06f5a) Co-authored-by: Ashwin P Chandran <[email protected]>
…ct#2227) Signed-off-by: Ashwin Pc <[email protected]> Signed-off-by: Sergey V. Osipov <[email protected]>
…ct#2227) Signed-off-by: Ashwin Pc <[email protected]> Signed-off-by: Sergey V. Osipov <[email protected]>
Signed-off-by: Ashwin Pc [email protected]
Description
[Describe what this change achieves]
Issues Resolved
#2221
Check List
yarn test:jestyarn test:jest_integrationyarn test:ftr