[GitHub] [superset] michael-s-molina commented on a change in pull request #16666: fix: Select refactoring known issues

[GitBox <gi...@apache.org>]

michael-s-molina commented on a change in pull request #16666:
URL: https://github.com/apache/superset/pull/16666#discussion_r707424184



##########
File path: superset-frontend/src/components/Select/Select.tsx
##########
@@ -458,18 +415,26 @@ const Select = ({
       return filterOption(search, option);
     }
 
+    let found = false;
+

Review comment:
       ```suggestion
   ```

##########
File path: superset-frontend/src/components/Select/Select.tsx
##########
@@ -563,12 +568,12 @@ const Select = ({
         onDeselect={handleOnDeselect}
         onDropdownVisibleChange={handleOnDropdownVisibleChange}
         onInputKeyDown={onInputKeyDown}
-        onPopupScroll={isAsync ? handlePagination : undefined}
-        onSearch={shouldShowSearch ? handleOnSearch : undefined}
+        onPopupScroll={(isAsync && handlePagination) || undefined}
+        onSearch={(shouldShowSearch && handleOnSearch) || undefined}
         onSelect={handleOnSelect}
         onClear={handleClear}
         onChange={onChange}
-        options={selectOptions}
+        options={(!shouldUseChildrenOptions && selectOptions) || undefined}

Review comment:
       Can we use the ternary operator here? It's more readable.

##########
File path: superset-frontend/src/components/Select/Select.tsx
##########
@@ -563,12 +568,12 @@ const Select = ({
         onDeselect={handleOnDeselect}
         onDropdownVisibleChange={handleOnDropdownVisibleChange}
         onInputKeyDown={onInputKeyDown}
-        onPopupScroll={isAsync ? handlePagination : undefined}
-        onSearch={shouldShowSearch ? handleOnSearch : undefined}
+        onPopupScroll={(isAsync && handlePagination) || undefined}
+        onSearch={(shouldShowSearch && handleOnSearch) || undefined}

Review comment:
       I think the previous version is more readable.

##########
File path: superset-frontend/src/components/Select/Select.tsx
##########
@@ -458,18 +415,26 @@ const Select = ({
       return filterOption(search, option);
     }
 
+    let found = false;
+
     if (filterOption) {
       const searchValue = search.trim().toLowerCase();
-      const { value, label } = option;
-      const valueText = String(value);
-      const labelText = String(label);
-      return (
-        valueText.toLowerCase().includes(searchValue) ||
-        labelText.toLowerCase().includes(searchValue)
-      );
+
+      if (optionFilterProps && optionFilterProps.length) {
+        optionFilterProps.every(prop => {
+          const optionProp = option?.[prop]
+            ? String(option[prop]).trim().toLowerCase()
+            : '';
+          if (optionProp.includes(searchValue)) {
+            found = true;
+            return false;
+          }
+          return true;
+        });
+      }
     }
 
-    return false;
+    return found;

Review comment:
       ```suggestion
       return false;
   ```




-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org