From a413d6a81bc5e4ff384cdf3d4b16bd5c30b0e095 Mon Sep 17 00:00:00 2001
From: Anton Kulyk <kuliks.anton@gmail.com>
Date: Fri, 20 Oct 2023 12:41:18 +0100
Subject: [PATCH] Clean up QB filter panel components (#34852)
* Extract `FilterHeaderButton`
* Move `QuestionFilters` to its own directory
* Remove not used `QuestionFilters` component
* Extract styled components
* Convert `ViewPill` to TypeScript
* Convert `QuestionFilters` to TypeScript
* Clean up
* Fix label
---
.../components/view/FilterHeaderButton.tsx | 50 +++++
.../components/view/QuestionFilters.jsx | 190 ------------------
.../QuestionFilters.styled.tsx | 27 +++
.../view/QuestionFilters/QuestionFilters.tsx | 154 ++++++++++++++
.../components/view/QuestionFilters/index.ts | 1 +
.../components/view/ViewHeader.jsx | 17 +-
.../components/view/ViewHeader.styled.tsx | 24 ---
.../components/view/ViewHeader.unit.spec.js | 2 +-
.../view/{ViewPill.jsx => ViewPill.tsx} | 20 +-
9 files changed, 256 insertions(+), 229 deletions(-)
create mode 100644 frontend/src/metabase/query_builder/components/view/FilterHeaderButton.tsx
delete mode 100644 frontend/src/metabase/query_builder/components/view/QuestionFilters.jsx
create mode 100644 frontend/src/metabase/query_builder/components/view/QuestionFilters/QuestionFilters.styled.tsx
create mode 100644 frontend/src/metabase/query_builder/components/view/QuestionFilters/QuestionFilters.tsx
create mode 100644 frontend/src/metabase/query_builder/components/view/QuestionFilters/index.ts
rename frontend/src/metabase/query_builder/components/view/{ViewPill.jsx => ViewPill.tsx} (71%)
diff --git a/frontend/src/metabase/query_builder/components/view/FilterHeaderButton.tsx b/frontend/src/metabase/query_builder/components/view/FilterHeaderButton.tsx
new file mode 100644
index 00000000000..2869855f0eb
--- /dev/null
+++ b/frontend/src/metabase/query_builder/components/view/FilterHeaderButton.tsx
@@ -0,0 +1,50 @@
+import { t } from "ttag";
+import { color } from "metabase/lib/colors";
+import { MODAL_TYPES } from "metabase/query_builder/constants";
+import type { QueryBuilderMode } from "metabase-types/store";
+import type Question from "metabase-lib/Question";
+import { HeaderButton } from "./ViewHeader.styled";
+
+interface FilterHeaderButtonProps {
+ className?: string;
+ onOpenModal: (modalType: string) => void;
+}
+
+export function FilterHeaderButton({
+ className,
+ onOpenModal,
+}: FilterHeaderButtonProps) {
+ return (
+ <HeaderButton
+ className={className}
+ active={false}
+ large
+ labelBreakpoint="sm"
+ color={color("filter")}
+ onClick={() => onOpenModal(MODAL_TYPES.FILTERS)}
+ data-metabase-event="View Mode; Open Filter Modal"
+ data-testid="question-filter-header"
+ >
+ {t`Filter`}
+ </HeaderButton>
+ );
+}
+
+interface RenderCheckOpts {
+ question: Question;
+ queryBuilderMode: QueryBuilderMode;
+ isObjectDetail: boolean;
+ isActionListVisible: boolean;
+}
+
+FilterHeaderButton.shouldRender = ({
+ question,
+ queryBuilderMode,
+ isObjectDetail,
+ isActionListVisible,
+}: RenderCheckOpts) =>
+ queryBuilderMode === "view" &&
+ question.isStructured() &&
+ question.query().isEditable() &&
+ !isObjectDetail &&
+ isActionListVisible;
diff --git a/frontend/src/metabase/query_builder/components/view/QuestionFilters.jsx b/frontend/src/metabase/query_builder/components/view/QuestionFilters.jsx
deleted file mode 100644
index fc3d7693684..00000000000
--- a/frontend/src/metabase/query_builder/components/view/QuestionFilters.jsx
+++ /dev/null
@@ -1,190 +0,0 @@
-/* eslint-disable react/prop-types */
-import { t } from "ttag";
-
-import Tooltip from "metabase/core/components/Tooltip";
-import PopoverWithTrigger from "metabase/components/PopoverWithTrigger";
-
-import { MODAL_TYPES } from "metabase/query_builder/constants";
-import { FilterPopover } from "metabase/query_builder/components/filters/FilterPopover";
-import { color } from "metabase/lib/colors";
-import ViewPill from "./ViewPill";
-
-import {
- HeaderButton,
- FilterHeaderContainer,
- FilterHeaderButton,
-} from "./ViewHeader.styled";
-
-const FilterPill = props => <ViewPill color={color("filter")} {...props} />;
-
-export default function QuestionFilters({
- className,
- question,
- expanded,
- onExpand,
- onCollapse,
- onQueryChange,
-}) {
- const query = question.query();
- const filters = query.topLevelFilters();
- if (filters.length === 0) {
- return null;
- }
-
- return (
- <div className={className}>
- <div className="flex flex-wrap align-center mbn1 mrn1">
- <Tooltip tooltip={expanded ? t`Hide filters` : t`Show filters`}>
- <FilterPill
- invert
- icon="filter"
- className="text-small mr1 mb1 cursor-pointer"
- onClick={expanded ? onCollapse : onExpand}
- data-metabase-event={
- expanded
- ? `View Mode; Header Filters Collapse Click`
- : `View Mode; Header Filters Expand Click`
- }
- data-testid="filters-visibility-control"
- >
- {expanded ? null : filters.length}
- </FilterPill>
- </Tooltip>
- {expanded &&
- filters.map((filter, index) => (
- <PopoverWithTrigger
- key={index}
- triggerElement={
- <FilterPill
- onRemove={() => onQueryChange(filter.remove().rootQuery())}
- >
- {filter.displayName()}
- </FilterPill>
- }
- triggerClasses="flex flex-no-shrink align-center mr1 mb1"
- sizeToFit
- >
- <FilterPopover
- isTopLevel
- query={query}
- filter={filter}
- onChangeFilter={newFilter =>
- onQueryChange(newFilter.replace().rootQuery())
- }
- className="scroll-y"
- />
- </PopoverWithTrigger>
- ))}
- </div>
- </div>
- );
-}
-
-export function FilterHeaderToggle({
- className,
- question,
- onExpand,
- expanded,
- onCollapse,
- onQueryChange,
-}) {
- const query = question.query();
- const filters = query.topLevelFilters();
- if (filters.length === 0) {
- return null;
- }
- return (
- <div className={className}>
- <Tooltip tooltip={expanded ? t`Hide filters` : t`Show filters`}>
- <FilterHeaderButton
- small
- rounded
- icon="filter"
- onClick={expanded ? onCollapse : onExpand}
- active={expanded}
- data-testid="filters-visibility-control"
- >
- <span>{filters.length}</span>
- </FilterHeaderButton>
- </Tooltip>
- </div>
- );
-}
-
-export function FilterHeader({ question, expanded, onQueryChange }) {
- const query = question.query();
- const filters = query.topLevelFilters();
- if (filters.length === 0 || !expanded) {
- return null;
- }
- return (
- <FilterHeaderContainer data-testid="qb-filters-panel">
- <div className="flex flex-wrap align-center">
- {filters.map((filter, index) => (
- <PopoverWithTrigger
- key={index}
- triggerElement={
- <FilterPill
- onRemove={() => onQueryChange(filter.remove().rootQuery())}
- >
- {filter.displayName()}
- </FilterPill>
- }
- triggerClasses="flex flex-no-shrink align-center mr1 mb1"
- sizeToFit
- >
- <FilterPopover
- isTopLevel
- query={query}
- filter={filter}
- onChangeFilter={newFilter =>
- onQueryChange(newFilter.replace().rootQuery())
- }
- className="scroll-y"
- />
- </PopoverWithTrigger>
- ))}
- </div>
- </FilterHeaderContainer>
- );
-}
-
-export function QuestionFilterWidget({ onOpenModal, className }) {
- return (
- <HeaderButton
- large
- labelBreakpoint="sm"
- className={className}
- color={color("filter")}
- onClick={() => onOpenModal(MODAL_TYPES.FILTERS)}
- aria-label={t`Show more filters`}
- data-metabase-event="View Mode; Open Filter Modal"
- data-testid="question-filter-header"
- >
- {t`Filter`}
- </HeaderButton>
- );
-}
-
-QuestionFilters.shouldRender = ({
- question,
- queryBuilderMode,
- isObjectDetail,
-}) =>
- queryBuilderMode === "view" &&
- question.isStructured() &&
- question.query().isEditable() &&
- question.query().topLevelFilters().length > 0 &&
- !isObjectDetail;
-
-QuestionFilterWidget.shouldRender = ({
- question,
- queryBuilderMode,
- isObjectDetail,
- isActionListVisible,
-}) =>
- queryBuilderMode === "view" &&
- question.isStructured() &&
- question.query().isEditable() &&
- !isObjectDetail &&
- isActionListVisible;
diff --git a/frontend/src/metabase/query_builder/components/view/QuestionFilters/QuestionFilters.styled.tsx b/frontend/src/metabase/query_builder/components/view/QuestionFilters/QuestionFilters.styled.tsx
new file mode 100644
index 00000000000..6f9cad49827
--- /dev/null
+++ b/frontend/src/metabase/query_builder/components/view/QuestionFilters/QuestionFilters.styled.tsx
@@ -0,0 +1,27 @@
+import styled from "@emotion/styled";
+import Button from "metabase/core/components/Button";
+import { color, alpha } from "metabase/lib/colors";
+import { space } from "metabase/styled-components/theme";
+
+export const FilterHeaderContainer = styled.div`
+ padding: ${space(1)} ${space(3)} 0 ${space(3)};
+ border-bottom: 1px solid ${color("border")};
+`;
+
+export const FilterHeaderButton = styled(Button)<{ active: boolean }>`
+ background-color: ${({ active }) =>
+ active ? alpha(color("filter"), 0.8) : alpha(color("filter"), 0.2)};
+ color: ${({ active }) => (active ? "white" : color("filter"))};
+ border-radius: 99px;
+ padding-top: ${space(0.5)};
+ padding-bottom: ${space(0.5)};
+ &:hover {
+ background-color: ${color("filter")};
+ color: white;
+ }
+ transition: background 300ms linear, border 300ms linear;
+
+ @media (prefers-reduced-motion) {
+ transition: none;
+ }
+`;
diff --git a/frontend/src/metabase/query_builder/components/view/QuestionFilters/QuestionFilters.tsx b/frontend/src/metabase/query_builder/components/view/QuestionFilters/QuestionFilters.tsx
new file mode 100644
index 00000000000..35a8f91694c
--- /dev/null
+++ b/frontend/src/metabase/query_builder/components/view/QuestionFilters/QuestionFilters.tsx
@@ -0,0 +1,154 @@
+import { t } from "ttag";
+
+import { Flex, Tooltip } from "metabase/ui";
+import PopoverWithTrigger from "metabase/components/PopoverWithTrigger";
+import { FilterPopover } from "metabase/query_builder/components/filters/FilterPopover";
+
+import { color } from "metabase/lib/colors";
+
+import type { QueryBuilderMode } from "metabase-types/store";
+import type Question from "metabase-lib/Question";
+import type StructuredQuery from "metabase-lib/queries/StructuredQuery";
+import type Filter from "metabase-lib/queries/structured/Filter";
+
+import ViewPill from "../ViewPill";
+import type { ViewPillProps } from "../ViewPill";
+import {
+ FilterHeaderContainer,
+ FilterHeaderButton,
+} from "./QuestionFilters.styled";
+
+const FilterPill = (props: ViewPillProps) => (
+ <ViewPill color={color("filter")} {...props} />
+);
+
+interface FilterHeaderToggleProps {
+ className?: string;
+ question: Question;
+ expanded: boolean;
+ onExpand: () => void;
+ onCollapse: () => void;
+}
+
+export function FilterHeaderToggle({
+ className,
+ question,
+ expanded,
+ onExpand,
+ onCollapse,
+}: FilterHeaderToggleProps) {
+ const query = question.query() as StructuredQuery;
+ const filters = query.topLevelFilters();
+ return (
+ <div className={className}>
+ <Tooltip label={expanded ? t`Hide filters` : t`Show filters`}>
+ <FilterHeaderButton
+ small
+ icon="filter"
+ onClick={expanded ? onCollapse : onExpand}
+ active={expanded}
+ data-metabase-event={
+ expanded
+ ? `View Mode; Header Filters Collapse Click`
+ : `View Mode; Header Filters Expand Click`
+ }
+ data-testid="filters-visibility-control"
+ >
+ <span>{filters.length}</span>
+ </FilterHeaderButton>
+ </Tooltip>
+ </div>
+ );
+}
+
+interface FilterHeaderProps {
+ question: Question;
+ expanded: boolean;
+ onQueryChange: (query: StructuredQuery) => void;
+}
+
+export function FilterHeader({
+ question,
+ expanded,
+ onQueryChange,
+}: FilterHeaderProps) {
+ const query = question.query() as StructuredQuery;
+ const filters = query.topLevelFilters();
+
+ if (filters.length === 0 || !expanded) {
+ return null;
+ }
+
+ return (
+ <FilterHeaderContainer data-testid="qb-filters-panel">
+ <Flex align="center" wrap="wrap">
+ {filters.map((filter, index) => (
+ <FilterHeaderPopover
+ key={index}
+ query={query}
+ filter={filter}
+ onQueryChange={onQueryChange}
+ />
+ ))}
+ </Flex>
+ </FilterHeaderContainer>
+ );
+}
+
+interface FilterHeaderPopoverProps {
+ query: StructuredQuery;
+ filter: Filter;
+ onQueryChange: (query: StructuredQuery) => void;
+}
+
+function FilterHeaderPopover({
+ query,
+ filter,
+ onQueryChange,
+}: FilterHeaderPopoverProps) {
+ const handleChange = (newFilter: Filter) => {
+ onQueryChange(newFilter.replace().rootQuery());
+ };
+
+ const handleRemove = () => {
+ onQueryChange(filter.remove().rootQuery());
+ };
+
+ return (
+ <PopoverWithTrigger
+ triggerElement={
+ <FilterPill onRemove={handleRemove}>{filter.displayName()}</FilterPill>
+ }
+ triggerClasses="flex flex-no-shrink align-center mr1 mb1"
+ sizeToFit
+ >
+ <FilterPopover
+ className="scroll-y"
+ query={query}
+ filter={filter}
+ isTopLevel
+ onChangeFilter={handleChange}
+ />
+ </PopoverWithTrigger>
+ );
+}
+
+type RenderCheckOpts = {
+ question: Question;
+ queryBuilderMode: QueryBuilderMode;
+ isObjectDetail: boolean;
+};
+
+const shouldRender = ({
+ question,
+ queryBuilderMode,
+ isObjectDetail,
+}: RenderCheckOpts) =>
+ queryBuilderMode === "view" &&
+ question.isStructured() &&
+ question.query().isEditable() &&
+ (question.query() as StructuredQuery).topLevelFilters().length > 0 &&
+ !isObjectDetail;
+
+FilterHeader.shouldRender = shouldRender;
+FilterHeaderToggle.shouldRender = shouldRender;
diff --git a/frontend/src/metabase/query_builder/components/view/QuestionFilters/index.ts b/frontend/src/metabase/query_builder/components/view/QuestionFilters/index.ts
new file mode 100644
index 00000000000..39f5ca9cb2c
--- /dev/null
+++ b/frontend/src/metabase/query_builder/components/view/QuestionFilters/index.ts
@@ -0,0 +1 @@
+export * from "./QuestionFilters";
diff --git a/frontend/src/metabase/query_builder/components/view/ViewHeader.jsx b/frontend/src/metabase/query_builder/components/view/ViewHeader.jsx
index adb7bd4a09e..700b5419554 100644
--- a/frontend/src/metabase/query_builder/components/view/ViewHeader.jsx
+++ b/frontend/src/metabase/query_builder/components/view/ViewHeader.jsx
@@ -19,16 +19,13 @@ import { MODAL_TYPES } from "metabase/query_builder/constants";
import { getDashboard } from "metabase/query_builder/selectors";
import * as ML_Urls from "metabase-lib/urls";
import QuestionActions from "../QuestionActions";
+import { FilterHeaderButton } from "./FilterHeaderButton";
import { HeadBreadcrumbs } from "./HeaderBreadcrumbs";
import QuestionDataSource from "./QuestionDataSource";
import QuestionDescription from "./QuestionDescription";
import { QuestionNotebookButton } from "./QuestionNotebookButton";
import ConvertQueryButton from "./ConvertQueryButton";
-import QuestionFilters, {
- FilterHeaderToggle,
- FilterHeader,
- QuestionFilterWidget,
-} from "./QuestionFilters";
+import { FilterHeaderToggle, FilterHeader } from "./QuestionFilters";
import { QuestionSummarizeWidget } from "./QuestionSummaries";
import {
AdHocViewHeading,
@@ -150,7 +147,7 @@ export function ViewTitleHeader(props) {
onQueryChange={onQueryChange}
/>
</ViewHeaderContainer>
- {QuestionFilters.shouldRender(props) && (
+ {FilterHeader.shouldRender(props) && (
<FilterHeader
{...props}
expanded={areFiltersExpanded}
@@ -415,7 +412,6 @@ function ViewTitleHeaderRightSide(props) {
onCloseQuestionInfo,
onOpenQuestionInfo,
onModelPersistenceChange,
- onQueryChange,
} = props;
const isShowingNotebook = queryBuilderMode === "notebook";
const query = question.query();
@@ -456,18 +452,17 @@ function ViewTitleHeaderRightSide(props) {
return (
<ViewHeaderActionPanel data-testid="qb-header-action-panel">
- {QuestionFilters.shouldRender(props) && (
+ {FilterHeaderToggle.shouldRender(props) && (
<FilterHeaderToggle
className="ml2 mr1"
question={question}
expanded={areFiltersExpanded}
onExpand={onExpandFilters}
onCollapse={onCollapseFilters}
- onQueryChange={onQueryChange}
/>
)}
- {QuestionFilterWidget.shouldRender(props) && (
- <QuestionFilterWidget
+ {FilterHeaderButton.shouldRender(props) && (
+ <FilterHeaderButton
className="hide sm-show"
onOpenModal={onOpenModal}
/>
diff --git a/frontend/src/metabase/query_builder/components/view/ViewHeader.styled.tsx b/frontend/src/metabase/query_builder/components/view/ViewHeader.styled.tsx
index 3ef9d25231d..2e4c4456282 100644
--- a/frontend/src/metabase/query_builder/components/view/ViewHeader.styled.tsx
+++ b/frontend/src/metabase/query_builder/components/view/ViewHeader.styled.tsx
@@ -102,31 +102,7 @@ export const IconHeaderButton = styled(HeaderButton)`
padding-right: 0.75rem;
`;
-export const FilterHeaderButton = styled(Button)<{ active: boolean }>`
- background-color: ${({ active }) =>
- active ? alpha(color("filter"), 0.8) : alpha(color("filter"), 0.2)};
- color: ${({ active }) => (active ? "white" : color("filter"))};
- border-radius: 99px;
- padding-top: ${space(0.5)};
- padding-bottom: ${space(0.5)};
- &:hover {
- background-color: ${color("filter")};
- color: white;
- }
- transition: background 300ms linear, border 300ms linear;
-
- @media (prefers-reduced-motion) {
- transition: none;
- }
-`;
-
const getDefaultColor = () => color("brand");
-
-export const FilterHeaderContainer = styled.div`
- padding: ${space(1)} ${space(3)} 0 ${space(3)};
- border-bottom: 1px solid ${color("border")};
-`;
-
export const StyledLastEditInfoLabel = styled(LastEditInfoLabel)`
color: ${color("text-light")};
//margin-left: 4px;
diff --git a/frontend/src/metabase/query_builder/components/view/ViewHeader.unit.spec.js b/frontend/src/metabase/query_builder/components/view/ViewHeader.unit.spec.js
index 1c2a6f20aaf..bfe576ab37b 100644
--- a/frontend/src/metabase/query_builder/components/view/ViewHeader.unit.spec.js
+++ b/frontend/src/metabase/query_builder/components/view/ViewHeader.unit.spec.js
@@ -252,7 +252,7 @@ describe("ViewHeader", () => {
setup({ card, database: null });
expect(screen.queryByText("Filter")).not.toBeInTheDocument();
expect(
- screen.queryByLabelText("Show more filters"),
+ screen.queryByTestId("filters-visibility-control"),
).not.toBeInTheDocument();
expect(screen.queryByText("Summarize")).not.toBeInTheDocument();
expect(
diff --git a/frontend/src/metabase/query_builder/components/view/ViewPill.jsx b/frontend/src/metabase/query_builder/components/view/ViewPill.tsx
similarity index 71%
rename from frontend/src/metabase/query_builder/components/view/ViewPill.jsx
rename to frontend/src/metabase/query_builder/components/view/ViewPill.tsx
index f647e4ebfce..62dad94b60b 100644
--- a/frontend/src/metabase/query_builder/components/view/ViewPill.jsx
+++ b/frontend/src/metabase/query_builder/components/view/ViewPill.tsx
@@ -1,9 +1,20 @@
-/* eslint-disable react/prop-types */
import cx from "classnames";
import { Icon } from "metabase/core/components/Icon";
+import type { IconName } from "metabase/core/components/Icon";
import { color as c, alpha } from "metabase/lib/colors";
-export default function ViewPill({
+export interface ViewPillProps {
+ className?: string;
+ color?: string;
+ invert?: boolean;
+ icon?: IconName;
+ children?: React.ReactNode;
+ style?: React.CSSProperties;
+ onClick?: React.MouseEventHandler<HTMLSpanElement>;
+ onRemove?: () => void;
+}
+
+function ViewPill({
className,
style = {},
color = c("brand"),
@@ -13,7 +24,7 @@ export default function ViewPill({
onRemove,
icon,
...props
-}) {
+}: ViewPillProps) {
return (
<span
{...props}
@@ -49,3 +60,6 @@ export default function ViewPill({
</span>
);
}
+
+// eslint-disable-next-line import/no-default-export
+export default ViewPill;
--
GitLab