[GitHub] [superset] kgabryje commented on a diff in pull request #22169: feat: Make filters and dividers display horizontally in horizontal native filters filter bar

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

kgabryje commented on code in PR #22169:
URL: https://github.com/apache/superset/pull/22169#discussion_r1031709497


##########
superset-frontend/src/dashboard/components/nativeFilters/FilterBar/FilterControls/FilterControl.tsx:
##########
@@ -129,27 +231,60 @@ const FilterControl: React.FC<FilterProps> = ({
   );
   const isRequired = !!filter.controlValues?.enableEmptyFilter;
 
+  const {
+    FilterControlContainer,
+    FormItem,
+    FilterControlTitleBox,
+    FilterControlTitle,
+  } = useFilterControlDisplay(orientation, overflow);
+
   const label = useMemo(
     () => (
-      <StyledFilterControlTitleBox>
-        <StyledFilterControlTitle data-test="filter-control-name">
+      <FilterControlTitleBox>
+        <FilterControlTitle data-test="filter-control-name">
           {name}
-        </StyledFilterControlTitle>
+        </FilterControlTitle>
         {isRequired && <RequiredFieldIndicator />}
         {filter.description?.trim() && (
           <DescriptionToolTip description={filter.description} />
         )}
         <StyledIcon data-test="filter-icon">{icon}</StyledIcon>
-      </StyledFilterControlTitleBox>
+      </FilterControlTitleBox>
     ),
-    [name, isRequired, filter.description, icon],
+    [
+      FilterControlTitleBox,
+      FilterControlTitle,
+      name,
+      isRequired,
+      filter.description,
+      icon,
+    ],
   );
 
   const isScrolling = useContext(FilterBarScrollContext);
+  const filterCardPlacement = useMemo(() => {
+    if (orientation === FilterBarOrientation.HORIZONTAL) {
+      if (overflow) {
+        return FilterCardPlacement.Left;
+      }
+      return FilterCardPlacement.Bottom;
+    }
+    return FilterCardPlacement.Right;
+  }, [orientation, overflow]);
 
   return (
-    <StyledFilterControlContainer layout="vertical">
-      <FilterCard filter={filter} isVisible={!isFilterActive && !isScrolling}>
+    <FilterControlContainer
+      layout={
+        orientation === FilterBarOrientation.HORIZONTAL && !overflow
+          ? 'horizontal'

Review Comment:
   nit: should we use an enum for `horizontal` and `vertical` layout? Maybe we could even simply use `FilterBarOrientation` for that? 



-- 
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