Skip to content

fix: nan issue when typing incomplete scientific notation #2016

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.

Sign up for GitHub

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

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

fix: nan issue when typing incomplete scientific notation #2016

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
57 changes: 38 additions & 19 deletions src/app/datasets/datasets-filter/datasets-filter.component.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -88,6 +88,8 @@ export class DatasetsFilterComponent implements OnInit, OnDestroy {

fieldTypeMap: { [key: string]: string } = {};

tempConditionValues: string[] = [];

constructor(
public appConfigService: AppConfigService,
public dialog: MatDialog,
Expand Down Expand Up @@ -229,15 +231,47 @@ export class DatasetsFilterComponent implements OnInit, OnDestroy {
});

this.conditionConfigs$.pipe(take(1)).subscribe((conditionConfigs) => {
(conditionConfigs || []).forEach((oldCondition) => {
const updatedConditions = (conditionConfigs || []).map((config, i) => {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

issue (complexity): Consider extracting the condition transformation logic into a helper function to improve readability and testability.

Here’s one way to collapse that nested‐branching and spread logic into a single helper, then use it inside your subscription. This keeps exactly the same behavior but is much easier to read and test.

// add somewhere in your component (or in a util file)
private transformConfigWithTempValue(
  config: ConditionConfig, 
  tempVal?: string
): ConditionConfig {
  if (tempVal === undefined) {
    return config;
  }

  const isNumeric = tempVal !== '' && !isNaN(Number(tempVal));
  const newCondition: ScientificCondition = {
    ...config.condition,
    rhs: isNumeric ? Number(tempVal) : tempVal,
    // if it was previously EQUAL_TO(_NUMERIC|_STRING), we overwrite;
    // otherwise we just keep the old relation
    relation: isNumeric
      ? 'EQUAL_TO_NUMERIC'
      : 'EQUAL_TO_STRING',
  };

  return { ...config, condition: newCondition };
}

Then in your subscription:

this.conditionConfigs$.pipe(take(1)).subscribe((conditionConfigs) => {
  const updatedConditions = (conditionConfigs || []).map((cfg, idx) =>
    this.transformConfigWithTempValue(cfg, this.tempConditionValues[idx])
  );

  // remove old, add new, update settings, fetch…
  updatedConditions.forEach(c =>
    this.store.dispatch(removeScientificConditionAction({ condition: c.condition }))
  );
  updatedConditions.forEach(c => {
    if (c.enabled && c.condition.lhs && c.condition.rhs) {
      this.store.dispatch(addScientificConditionAction({ condition: c.condition }));
    }
  });
  this.store.dispatch(updateUserSettingsAction({
    property: { conditions: updatedConditions },
  }));
  this.store.dispatch(fetchDatasetsAction());
  this.store.dispatch(fetchFacetCountsAction());
});

• Extracting the mapping makes the transformation logic reusable and removes all the nested if/else and重复 spread blocks.
• You can now unit‐test transformConfigWithTempValue in isolation without wiring up the whole RxJS pipeline.

if (this.tempConditionValues[i] !== undefined) {
const value = this.tempConditionValues[i];
const isNumeric = value !== "" && !isNaN(Number(value));
if (
config.condition.relation === "EQUAL_TO" ||
config.condition.relation === "EQUAL_TO_NUMERIC" ||
config.condition.relation === "EQUAL_TO_STRING"
) {
return {
...config,
condition: {
...config.condition,
rhs: isNumeric ? Number(value) : value,
relation: isNumeric
? ("EQUAL_TO_NUMERIC" as ScientificCondition["relation"])
Comment on lines +248 to +249
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nitpick: Type assertion for relation may be unnecessary if type inference is sufficient.

Remove the type assertion if the assignment is already type-safe and the type checker does not require it.

: ("EQUAL_TO_STRING" as ScientificCondition["relation"]),
},
};
} else {
return {
...config,
condition: {
...config.condition,
rhs: isNumeric ? Number(value) : value,
},
};
}
}
return config;
});

updatedConditions.forEach((oldCondition) => {
this.store.dispatch(
removeScientificConditionAction({
condition: oldCondition.condition,
}),
);
});
Comment on lines 271 to 272
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

suggestion: Variable naming: 'oldCondition' is misleading after mapping to updatedConditions.

Please rename 'oldCondition' to 'condition' or 'updatedCondition' to better reflect its current meaning.

Suggested change
);
});
updatedConditions.forEach((condition) => {
this.store.dispatch(
removeScientificConditionAction({
condition: condition.condition,
);
});


(conditionConfigs || []).forEach((config) => {
updatedConditions.forEach((config) => {
if (config.enabled && config.condition.lhs && config.condition.rhs) {
this.store.dispatch(
addScientificConditionAction({ condition: config.condition }),
Expand All @@ -247,7 +281,7 @@ export class DatasetsFilterComponent implements OnInit, OnDestroy {

this.store.dispatch(
updateUserSettingsAction({
property: { conditions: conditionConfigs },
property: { conditions: updatedConditions },
}),
);
this.store.dispatch(fetchDatasetsAction());
Expand Down Expand Up @@ -623,22 +657,7 @@ export class DatasetsFilterComponent implements OnInit, OnDestroy {

updateConditionValue(index: number, event: Event) {
const newValue = (event.target as HTMLInputElement).value;
const currentRelation = this.asyncPipe.transform(this.conditionConfigs$)?.[
index
]?.condition.relation;
if (
currentRelation === "EQUAL_TO" ||
currentRelation === "EQUAL_TO_NUMERIC" ||
currentRelation === "EQUAL_TO_STRING"
) {
const isNumeric = newValue !== "" && !isNaN(Number(newValue));
this.updateConditionField(index, {
rhs: isNumeric ? Number(newValue) : newValue,
relation: isNumeric ? "EQUAL_TO_NUMERIC" : "EQUAL_TO_STRING",
});
} else {
this.updateConditionField(index, { rhs: Number(newValue) });
}
this.tempConditionValues[index] = newValue;
}

updateConditionRangeValue(index: number, event: Event, rangeIndex: 0 | 1) {
Expand Down
Loading