[GitHub] [superset] codyml commented on a diff in pull request #22064: feat: Horizontal filter bar states

[GitBox]

codyml commented on code in PR #22064:
URL: https://github.com/apache/superset/pull/22064#discussion_r1018328967


##########
superset-frontend/src/dashboard/components/nativeFilters/FilterBar/FilterBarLocationSelect/index.tsx:
##########
@@ -62,7 +62,7 @@ export const FilterBarLocationSelect = () => {
   return (
     <DropdownSelectableIcon

Review Comment:
   It looks like this component gives itself a line height that stops it from 
looking quite vertically aligned in the flex container – would it be possible 
to override that in horizontal mode?



##########
superset-frontend/src/dashboard/components/nativeFilters/FilterBar/Horizontal.tsx:
##########
@@ -0,0 +1,130 @@
+/**
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements.  See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership.  The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License.  You may obtain a copy of the License at
+ *
+ *   http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied.  See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+
+import React from 'react';
+import { styled, t } from '@superset-ui/core';
+import Icons from 'src/components/Icons';
+import Loading from 'src/components/Loading';
+import FilterControls from './FilterControls/FilterControls';
+import { getFilterBarTestId, HorizontalBarProps } from './utils';
+import FilterBarLocationSelect from './FilterBarLocationSelect';
+import FilterConfigurationLink from './FilterConfigurationLink';
+
+const HorizontalBar = styled.div`
+  ${({ theme }) => `
+  padding: ${theme.gridUnit * 2}px ${theme.gridUnit * 2}px;
+  background: ${theme.colors.grayscale.light5};
+  box-shadow: inset 0px -2px 2px -1px ${theme.colors.grayscale.light2};
+`}
+`;
+
+const HorizontalBarContent = styled.div`
+  ${({ theme }) => `
+  display: flex;
+  flex-direction: row;
+  flex-wrap: nowrap;
+  align-items: center;
+  justify-content: flex-start;
+  padding: 0 ${theme.gridUnit * 2}px;
+
+  .loading {
+    margin: ${theme.gridUnit * 2}px auto ${theme.gridUnit * 2}px;
+    padding: 0;
+  }
+`}
+`;
+
+const FilterBarEmptyStateContainer = styled.div`
+  ${({ theme }) => `
+  margin: 0 ${theme.gridUnit * 2}px;

Review Comment:
   Two design nits:
   - I think according to the designs, distance between the gear and this 
message should be 18px.
   - It looks like it should be size 12px (`s`) and color #666666 
(`greyscale.base`)



##########
superset-frontend/src/dashboard/components/nativeFilters/FilterBar/Horizontal.tsx:
##########
@@ -0,0 +1,130 @@
+/**
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements.  See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership.  The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License.  You may obtain a copy of the License at
+ *
+ *   http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied.  See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+
+import React from 'react';
+import { styled, t } from '@superset-ui/core';
+import Icons from 'src/components/Icons';
+import Loading from 'src/components/Loading';
+import FilterControls from './FilterControls/FilterControls';
+import { getFilterBarTestId, HorizontalBarProps } from './utils';
+import FilterBarLocationSelect from './FilterBarLocationSelect';
+import FilterConfigurationLink from './FilterConfigurationLink';
+
+const HorizontalBar = styled.div`
+  ${({ theme }) => `
+  padding: ${theme.gridUnit * 2}px ${theme.gridUnit * 2}px;
+  background: ${theme.colors.grayscale.light5};
+  box-shadow: inset 0px -2px 2px -1px ${theme.colors.grayscale.light2};
+`}
+`;
+
+const HorizontalBarContent = styled.div`
+  ${({ theme }) => `
+  display: flex;
+  flex-direction: row;
+  flex-wrap: nowrap;
+  align-items: center;
+  justify-content: flex-start;
+  padding: 0 ${theme.gridUnit * 2}px;
+
+  .loading {
+    margin: ${theme.gridUnit * 2}px auto ${theme.gridUnit * 2}px;
+    padding: 0;
+  }
+`}
+`;
+
+const FilterBarEmptyStateContainer = styled.div`
+  ${({ theme }) => `
+  margin: 0 ${theme.gridUnit * 2}px;
+  font-weight: ${theme.typography.weights.bold};
+`}
+`;
+
+const FiltersLinkContainer = styled.div<{ hasFilters: boolean }>`
+  ${({ theme, hasFilters }) => `
+  padding: 0 ${theme.gridUnit * 2}px;
+  border-right: ${
+    hasFilters ? `1px solid ${theme.colors.grayscale.light2}` : 0
+  };
+
+  button {
+    display: flex;
+    align-items: center;
+    text-transform: capitalize;
+    font-weight: ${theme.typography.weights.normal};
+    color: ${theme.colors.primary.base};
+    > .anticon + span, > .anticon {
+        margin-right: 0;
+        margin-left: 0;
+      }
+  }
+`}
+`;
+
+const HorizontalFilterBar: React.FC<HorizontalBarProps> = ({
+  actions,
+  canEdit,
+  dashboardId,
+  dataMaskSelected,
+  filterValues,
+  isInitialized,
+  directPathToChild,
+  onSelectionChange,
+}) => {
+  const hasFilters = filterValues.length > 0;
+
+  return (
+    <HorizontalBar {...getFilterBarTestId()}>
+      <HorizontalBarContent>
+        {!isInitialized ? (
+          <Loading position="inline-centered" />
+        ) : (
+          <>
+            {canEdit && <FilterBarLocationSelect />}
+            {!hasFilters && (
+              <FilterBarEmptyStateContainer>
+                {t('No filters are currently added to this dashboard.')}
+              </FilterBarEmptyStateContainer>
+            )}
+            {canEdit && (
+              <FiltersLinkContainer hasFilters={hasFilters}>
+                <FilterConfigurationLink
+                  dashboardId={dashboardId}
+                  createNewOnOpen={filterValues.length === 0}
+                >
+                  <Icons.PlusSmall /> {t('Add/Edit Filters')}

Review Comment:
   Another design nit: I think the plus icon has some padding or something 
stopping it from being vertically-centered, and it looks maybe a little closer 
to the Add/Edit Filters text than it should (~12px).



##########
superset-frontend/src/dashboard/components/nativeFilters/FilterBar/FilterBarLocationSelect/index.tsx:
##########
@@ -62,7 +62,7 @@ export const FilterBarLocationSelect = () => {
   return (
     <DropdownSelectableIcon

Review Comment:
   As-is:
   <img width="608" alt="Screen Shot 2022-11-09 at 12 22 16 PM" 
src="https://user-images.githubusercontent.com/13007381/200922119-5f30afce-14e4-437a-83e3-d971ae957a05.png";>
   
   With `line-height: 0;`: 
   <img width="671" alt="Screen Shot 2022-11-09 at 12 23 00 PM" 
src="https://user-images.githubusercontent.com/13007381/200922209-b22a1532-1d7f-43ca-88c0-420c00eb6659.png";>
   



##########
superset-frontend/src/dashboard/components/nativeFilters/FilterBar/Horizontal.tsx:
##########
@@ -0,0 +1,130 @@
+/**
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements.  See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership.  The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License.  You may obtain a copy of the License at
+ *
+ *   http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied.  See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+
+import React from 'react';
+import { styled, t } from '@superset-ui/core';
+import Icons from 'src/components/Icons';
+import Loading from 'src/components/Loading';
+import FilterControls from './FilterControls/FilterControls';
+import { getFilterBarTestId, HorizontalBarProps } from './utils';
+import FilterBarLocationSelect from './FilterBarLocationSelect';
+import FilterConfigurationLink from './FilterConfigurationLink';
+
+const HorizontalBar = styled.div`
+  ${({ theme }) => `
+  padding: ${theme.gridUnit * 2}px ${theme.gridUnit * 2}px;
+  background: ${theme.colors.grayscale.light5};
+  box-shadow: inset 0px -2px 2px -1px ${theme.colors.grayscale.light2};
+`}
+`;
+
+const HorizontalBarContent = styled.div`
+  ${({ theme }) => `
+  display: flex;
+  flex-direction: row;
+  flex-wrap: nowrap;
+  align-items: center;
+  justify-content: flex-start;
+  padding: 0 ${theme.gridUnit * 2}px;
+
+  .loading {
+    margin: ${theme.gridUnit * 2}px auto ${theme.gridUnit * 2}px;
+    padding: 0;
+  }
+`}
+`;
+
+const FilterBarEmptyStateContainer = styled.div`
+  ${({ theme }) => `
+  margin: 0 ${theme.gridUnit * 2}px;

Review Comment:
   Design:
   
   <img width="358" alt="Screen Shot 2022-11-09 at 12 31 13 PM" 
src="https://user-images.githubusercontent.com/13007381/200923724-50ee233d-dfc1-4c79-a845-9bd014240176.png";>
   



##########
superset-frontend/src/dashboard/components/nativeFilters/FilterBar/Horizontal.tsx:
##########
@@ -0,0 +1,130 @@
+/**
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements.  See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership.  The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License.  You may obtain a copy of the License at
+ *
+ *   http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied.  See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+
+import React from 'react';
+import { styled, t } from '@superset-ui/core';
+import Icons from 'src/components/Icons';
+import Loading from 'src/components/Loading';
+import FilterControls from './FilterControls/FilterControls';
+import { getFilterBarTestId, HorizontalBarProps } from './utils';
+import FilterBarLocationSelect from './FilterBarLocationSelect';
+import FilterConfigurationLink from './FilterConfigurationLink';
+
+const HorizontalBar = styled.div`
+  ${({ theme }) => `
+  padding: ${theme.gridUnit * 2}px ${theme.gridUnit * 2}px;
+  background: ${theme.colors.grayscale.light5};
+  box-shadow: inset 0px -2px 2px -1px ${theme.colors.grayscale.light2};
+`}
+`;
+
+const HorizontalBarContent = styled.div`
+  ${({ theme }) => `
+  display: flex;
+  flex-direction: row;
+  flex-wrap: nowrap;
+  align-items: center;
+  justify-content: flex-start;
+  padding: 0 ${theme.gridUnit * 2}px;
+
+  .loading {
+    margin: ${theme.gridUnit * 2}px auto ${theme.gridUnit * 2}px;
+    padding: 0;
+  }
+`}
+`;
+
+const FilterBarEmptyStateContainer = styled.div`
+  ${({ theme }) => `
+  margin: 0 ${theme.gridUnit * 2}px;
+  font-weight: ${theme.typography.weights.bold};
+`}
+`;
+
+const FiltersLinkContainer = styled.div<{ hasFilters: boolean }>`
+  ${({ theme, hasFilters }) => `
+  padding: 0 ${theme.gridUnit * 2}px;
+  border-right: ${
+    hasFilters ? `1px solid ${theme.colors.grayscale.light2}` : 0
+  };
+
+  button {
+    display: flex;
+    align-items: center;
+    text-transform: capitalize;
+    font-weight: ${theme.typography.weights.normal};
+    color: ${theme.colors.primary.base};
+    > .anticon + span, > .anticon {
+        margin-right: 0;
+        margin-left: 0;
+      }
+  }
+`}
+`;
+
+const HorizontalFilterBar: React.FC<HorizontalBarProps> = ({
+  actions,
+  canEdit,
+  dashboardId,
+  dataMaskSelected,
+  filterValues,
+  isInitialized,
+  directPathToChild,
+  onSelectionChange,
+}) => {
+  const hasFilters = filterValues.length > 0;
+
+  return (
+    <HorizontalBar {...getFilterBarTestId()}>
+      <HorizontalBarContent>
+        {!isInitialized ? (
+          <Loading position="inline-centered" />
+        ) : (
+          <>
+            {canEdit && <FilterBarLocationSelect />}
+            {!hasFilters && (
+              <FilterBarEmptyStateContainer>
+                {t('No filters are currently added to this dashboard.')}
+              </FilterBarEmptyStateContainer>
+            )}
+            {canEdit && (
+              <FiltersLinkContainer hasFilters={hasFilters}>
+                <FilterConfigurationLink
+                  dashboardId={dashboardId}
+                  createNewOnOpen={filterValues.length === 0}
+                >
+                  <Icons.PlusSmall /> {t('Add/Edit Filters')}

Review Comment:
   Design:
   
   <img width="133" alt="Screen Shot 2022-11-09 at 12 33 37 PM" 
src="https://user-images.githubusercontent.com/13007381/200924236-18b1b70c-9feb-48ef-a3cd-71806d50a683.png";>
   



##########
superset-frontend/src/dashboard/components/nativeFilters/FilterBar/ActionButtons/index.tsx:
##########
@@ -37,38 +38,15 @@ interface ActionButtonsProps {
   dataMaskSelected: DataMaskState;
   dataMaskApplied: DataMaskStateWithId;
   isApplyDisabled: boolean;
+  orientation?: FilterBarLocation;

Review Comment:
   Can we choose one of position vs. placement vs. orientation vs. location and 
use that everywhere (Redux, props, filenames, etc)?  I think if we go with 
orientation, vertical and horizontal make sense as the options, but if we go 
with position/placement/location I think top and left make more sense as the 
options.



##########
superset-frontend/src/dashboard/components/DashboardBuilder/DashboardBuilder.tsx:
##########
@@ -354,6 +361,14 @@ const DashboardBuilder: FC<DashboardBuilderProps> = () => {
     ({ dropIndicatorProps }: { dropIndicatorProps: JsonObject }) => (
       <div>
         {!hideDashboardHeader && <DashboardHeader />}
+        {nativeFiltersEnabled &&
+          !editMode &&
+          filterBarLocation === FilterBarLocation.HORIZONTAL && (
+            <FilterBar
+              orientation={FilterBarLocation.HORIZONTAL}
+              directPathToChild={directPathToChild}

Review Comment:
   Getting a missing dependency warning about `directPathToChild`.



-- 
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-unsubscr...@superset.apache.org

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


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