Dheeraj_CTM_656 #212

dgavini · 2025-01-07T18:26:31Z

Added toggle functionality for a few fields in matching criteria.

mickey-ng · 2025-01-10T15:38:17Z

libs/ui/src/lib/components/CtimsCombinedWidget.tsx

+    };
+
+    const questionMarkStyle = `${styles['question-mark']} pi pi-question-circle`;
+    const labelValue = schema?.["ui:title"] || schema.title || 'Hugo Symbol';


schema will never be null since it's a prop, the question mark is not necessary

This component no longer exists, as this code is moved to the Autocomplete component.

mickey-ng · 2025-01-10T15:39:11Z

libs/ui/src/lib/components/CtimsCombinedWidget.tsx

+        lineHeight: '20px',
+        color: '#495057',
+    };
+


can these 2 styles go into the css file?

This component no longer exists, as this code is moved to the Autocomplete component

mickey-ng · 2025-01-10T15:41:49Z

libs/ui/src/lib/components/CtimsCombinedWidget.tsx

is this the AutoCompleteField widget? Can you rename the file name to be more appropriate?

Are there a lot of changes between this vs CtimsAutoCompleteComponent? can we just add the toggle to CtimsAutoCompleteComponent and have it shown controlled by a property

This component no longer exists, as this code is moved to the Autocomplete component.

mickey-ng · 2025-01-10T15:47:49Z

libs/ui/src/lib/components/forms/GenomicForm.tsx

Are there more code to check in? I don't see AutocompleteFieldWithToggle defined in CtimsCombinedWidget

This component no longer exists, as this code is moved to the Autocomplete component. So, I am using the autocomplete component now to display toggle.

mickey-ng · 2025-01-10T16:28:36Z

libs/ui/src/lib/custom-rjsf-templates/CtimsDropdownWithExcludeToggle.tsx

+  } = props;
+
+  const [valueState, setValueState] = useState(value?.replace('!', '') || '');
+  const [isNegated, setIsNegated] = useState(value?.startsWith('!') || false);


value.startsWith("!") will return a boolean, when state is boolean there's no need to check the value of a boolean to set a boolean value

mickey-ng · 2025-01-10T16:39:03Z

libs/ui/src/lib/custom-rjsf-templates/CtimsDropdownWithExcludeToggle.tsx

@@ -0,0 +1,118 @@
+import { WidgetProps } from '@rjsf/utils';
+import React, { useEffect, useState } from 'react';
+import styles from './CtimsInputWithExcludeToggle.module.css';


Avoid importing from other component's css class for maintainability. Generally the css file services its own module, if there's a change in CtimsInputWithExcludeToggle it will unknowingly affect other components. If it's global style, consider moving to the global style.css

I have created a new CSS file for CTIMSDropdownWithExcludeToggle.

mickey-ng · 2025-01-10T16:40:09Z

libs/ui/src/lib/custom-rjsf-templates/CtimsDropdownWithExcludeToggle.tsx

If there is not much difference with CtimsDropDown, consider consolidating into 1

There is a multiselect within the Dropdown component, which we don't need in CTIMSDropDownwithExcludeToggle. So its better we have separate components for both.

mickey-ng · 2025-01-10T16:43:48Z

libs/ui/src/lib/components/forms/GenomicForm.tsx

+          "!Heterozygous deletion",
+          "!Homozygous deletion",
+          "!Gain",
+          "!High level amplification",


Does this mean we need to have all negated values in the form? Can this be just read in memory and not specified in enum? there's no corresponding enumNames

In order to make sure the schema accepts a "!" value, we should add the negative values in the enum. I have added a check to make sure only the value without "!" is displayed in the UI in CtimsDropdownComponent.

mickey-ng · 2025-01-22T16:59:08Z

libs/ui/src/lib/components/CtimsAutoCompleteComponent.tsx

+  const [selectedHugoSymbol, setSelectedHugoSymbol] = useState<string>(props.value || '');
+  const [excludeToggle, setExcludeToggle] = useState<boolean>(
+    props.value?.startsWith('!') || false
+  );


Initial value don't need to be determined by props.value since their values will be updated in the useEffect below, Initiation value can be simply "" and false, and the useEffect hook will update accordinly

I have made changes to set selectedHugoSymbol to "" and excludeToggle to "false"

mickey-ng · 2025-01-22T17:04:13Z

libs/ui/src/lib/components/CtimsAutoCompleteComponent.tsx



 useEffect(() => {
-  setSelectedHugoSymbol([props.value]);
+  const isExcluded = props.value?.startsWith('!');


Use type to simplify this: const isExcluded: boolean
Then you don't need to use "|| false" below

Added Boolean type for isExcluded

mickey-ng · 2025-01-22T18:46:05Z

libs/ui/src/lib/custom-rjsf-templates/CtimsDropdownWithExcludeToggle.tsx

+  } = props;
+
+  const [valueState, setValueState] = useState(value?.replace('!', '') || '');
+  const [isNegated, setIsNegated] = useState(value?.startsWith('!'));


Usually we set the initial value as '' or false, then have a useEffect hook depending on value to update the state constant. Since they are declared as "const", their value shouldn't be changed, and we rely on react's hook to update the value. The current way of declaration is more a "let" than "const".

Initialized the valueState with "" and isNegated with false.

mickey-ng · 2025-01-22T18:53:51Z

libs/ui/src/lib/custom-rjsf-templates/CtimsDropdownWithExcludeToggle.tsx

+  };
+
+  const handleBlur = () => {
+    const trimmedValue = valueState?.trim?.();


If valueState was initialized as "" by default, then it avoids redundant check to see if valueState is defined and if trim method exists

Initialized valueState with "", so removed checked if valueState exists with in handleBlur function.

mickey-ng · 2025-01-22T18:59:33Z

libs/ui/src/lib/components/CtimsAutoCompleteComponent.tsx

Can you reformat this file? There's some inconsistent spacing

reformatted the complete file

Dheeraj has fixed based on the comment

mickey-ng · 2025-01-22T20:52:04Z

libs/ui/src/lib/custom-rjsf-templates/CtimsDropdownWithExcludeToggle.tsx

+  } = props;
+
+  let [valueState, setValueState] = useState(value?.replace('!', '') || '');
+  let [isNegated, setIsNegated] = useState(value?.startsWith('!'));


The proper way should use const, and leave the initial value as '' and false, and use a hook depending on the "value" to update the state.

The reason why I am replacing "!" with "" in the initial state is because in this particular scenario that enum has negative values and while displaying in the UI, I want to remove them and only show the regular values.

mickey-ng · 2025-01-22T21:01:46Z

libs/ui/src/lib/components/CtimsAutoCompleteComponent.tsx

+    const isExcluded : boolean = props.value?.startsWith('!');
+    setSelectedHugoSymbol(props.value ? props.value.replace('!', '') : '');
+    setExcludeToggle(isExcluded);
+  }, [props.value]);


A stylistic suggestion for the future: use an if at the top to set the context
if (props.value)
{...}
It makes reading the code easier by setting the context first
ie. If situation A, then the follow code happens, else the other code happens

Ok, I will keep it in mind for my future commits.

mickey-ng · 2025-01-22T21:23:12Z

libs/ui/src/lib/custom-rjsf-templates/CtimsDropdownWithExcludeToggle.tsx

+  useEffect(() => {
+    if (value) {
+      setValueState(value?.replace('!', ''));
+      setIsNegated(value?.startsWith('!'));


? is not necessary inside the if, it will always be defined

made code changes to remove ?.

mickey-ng · 2025-01-23T14:47:59Z

libs/ui/src/lib/custom-rjsf-templates/CtimsDropdownWithExcludeToggle.tsx

+    setValueState(selectedValue);
+    if (!selectedValue) {
+      setIsNegated(false);
+    }


This is the same as setIsNegated(selectedValue)? or do you only want to trigger this when selectedValue is false?

There is one case when the selected value is empty, let's say when a user selects an empty value in a dropdown, so this condition makes sure the taggle is disabled.

jagnathan · 2025-01-28T20:44:50Z

i reviewed the changes. looks good. lets split this into multiple PRs.

Added toggle functionality for few fields in matching criteria

0b0cc42

dgavini requested review from jagnathan and mickey-ng January 7, 2025 18:26

mickey-ng reviewed Jan 10, 2025

View reviewed changes

Made changes based on code Review

30f845f

dgavini requested a review from mickey-ng January 21, 2025 17:00

mickey-ng reviewed Jan 22, 2025

View reviewed changes

Code-Review Changes

f6a7585

mickey-ng reviewed Jan 22, 2025

View reviewed changes

Code-Review Changes

bbe647f

mickey-ng reviewed Jan 22, 2025

View reviewed changes

Code-Review Changes

53b93cd

mickey-ng reviewed Jan 23, 2025

View reviewed changes

Dheeraj_CTM_656 #212

Are you sure you want to change the base?

Dheeraj_CTM_656 #212

Conversation

dgavini commented Jan 7, 2025

mickey-ng Jan 10, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mickey-ng Jan 22, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jagnathan commented Jan 28, 2025 • edited Loading

mickey-ng Jan 10, 2025 •

edited

Loading

mickey-ng Jan 22, 2025 •

edited

Loading

jagnathan commented Jan 28, 2025 •

edited

Loading