From ef256e31086dd76ab744ab8898583369cf1dbc60 Mon Sep 17 00:00:00 2001
From: Benoit Vinay <ben@benoitvinay.com>
Date: Tue, 22 Mar 2022 20:56:19 +0100
Subject: [PATCH] Various fixes for errors seen in the console while browsing
(#21060)
* shouldNotForwardTransientProp method created for emotion
* BadgeIcon html attributes clean up
* Usage of transient property $hasMargin in Badge for BadgeIcon
* HTML attribute cleanup for LinkRoot
* React key added to ButtonBar child if not defined
* Properties for TableInfoPopover properly typed
* Misc
* analyticsContext property set as non required as non of the parents enforce it or pass a value
* Clean up attributes for SVG in Icon
* CSS fix for share icon special case in Icon
* Checkbox controlled component default value fix
* Misc clean up
* index file for collection side bar
* Collections collectionID type fix
* collection type in NewCollectionItemMenu fix
* Misc
* TokenField fix for value
* noPopover passed properly to WidgetStatusIcon
* Updates to controlled input/checkbox
* VS Code workspace file added to gitignore
* shouldNotForwardTransientProp renamed to shouldForwardNonTransientProp
* AddAggregationButton defaultProps updates
* propTypes for TableInfoPopover added back
---
.gitignore | 1 +
.../Collections/Collections.jsx | 2 +-
.../components/NewCollectionItemMenu.jsx | 10 +++++-----
.../containers/CollectionSidebar/index.ts | 1 +
frontend/src/metabase/components/Badge.jsx | 2 +-
.../src/metabase/components/Badge.styled.jsx | 7 +++++--
frontend/src/metabase/components/ButtonBar.jsx | 4 +++-
.../CollectionLanding/CollectionLanding.jsx | 6 +++---
frontend/src/metabase/components/Icon.tsx | 13 +++++++++----
.../TableInfoPopover/TableInfoPopover.tsx | 16 +++++++++-------
frontend/src/metabase/components/TokenField.jsx | 6 ++++--
.../core/components/CheckBox/CheckBox.tsx | 6 ++++--
.../core/components/Link/Link.styled.tsx | 5 ++++-
frontend/src/metabase/lib/styling/emotion.ts | 5 +++++
.../components/ParameterValueWidget.jsx | 4 ++--
.../AddAggregationButton.jsx | 2 +-
.../questions/components/CollectionBadge.jsx | 3 ++-
17 files changed, 60 insertions(+), 33 deletions(-)
create mode 100644 frontend/src/metabase/collections/containers/CollectionSidebar/index.ts
create mode 100644 frontend/src/metabase/lib/styling/emotion.ts
diff --git a/.gitignore b/.gitignore
index f2e037e9e25..c60848bbec3 100644
--- a/.gitignore
+++ b/.gitignore
@@ -80,3 +80,4 @@ dev/src/dev/nocommit/
.lsp/*
!.lsp/config.edn
.clj-kondo/.cache
+*.code-workspace
diff --git a/frontend/src/metabase/collections/components/CollectionSidebar/Collections/Collections.jsx b/frontend/src/metabase/collections/components/CollectionSidebar/Collections/Collections.jsx
index 071d3cf25d2..cae353b349f 100644
--- a/frontend/src/metabase/collections/components/CollectionSidebar/Collections/Collections.jsx
+++ b/frontend/src/metabase/collections/components/CollectionSidebar/Collections/Collections.jsx
@@ -10,7 +10,7 @@ import {
import { Container } from "./Collections.styled";
const propTypes = {
- collectionId: PropTypes.number,
+ collectionId: PropTypes.oneOfType([PropTypes.number, PropTypes.string]),
currentUserId: PropTypes.number,
handleToggleMobileSidebar: PropTypes.func.isRequired,
list: PropTypes.array,
diff --git a/frontend/src/metabase/collections/components/NewCollectionItemMenu.jsx b/frontend/src/metabase/collections/components/NewCollectionItemMenu.jsx
index 772e54fe7b7..1fe9df69969 100644
--- a/frontend/src/metabase/collections/components/NewCollectionItemMenu.jsx
+++ b/frontend/src/metabase/collections/components/NewCollectionItemMenu.jsx
@@ -2,13 +2,13 @@ import React from "react";
import PropTypes from "prop-types";
import { t } from "ttag";
-import * as Urls from "metabase/lib/urls";
+import { newQuestion, newDashboard, newCollection } from "metabase/lib/urls";
import EntityMenu from "metabase/components/EntityMenu";
import { ANALYTICS_CONTEXT } from "metabase/collections/constants";
const propTypes = {
- collection: PropTypes.func,
+ collection: PropTypes.object,
list: PropTypes.arrayOf(PropTypes.object),
};
@@ -17,19 +17,19 @@ function NewCollectionItemMenu({ collection }) {
{
icon: "insight",
title: t`Question`,
- link: Urls.newQuestion({ mode: "notebook", collectionId: collection.id }),
+ link: newQuestion({ mode: "notebook", collectionId: collection.id }),
event: `${ANALYTICS_CONTEXT};New Item Menu;Question Click`,
},
{
icon: "dashboard",
title: t`Dashboard`,
- link: Urls.newDashboard(collection.id),
+ link: newDashboard(collection.id),
event: `${ANALYTICS_CONTEXT};New Item Menu;Dashboard Click`,
},
{
icon: "folder",
title: t`Collection`,
- link: Urls.newCollection(collection.id),
+ link: newCollection(collection.id),
event: `${ANALYTICS_CONTEXT};New Item Menu;Collection Click`,
},
];
diff --git a/frontend/src/metabase/collections/containers/CollectionSidebar/index.ts b/frontend/src/metabase/collections/containers/CollectionSidebar/index.ts
new file mode 100644
index 00000000000..04f8db55b43
--- /dev/null
+++ b/frontend/src/metabase/collections/containers/CollectionSidebar/index.ts
@@ -0,0 +1 @@
+export { default } from "./CollectionSidebar";
diff --git a/frontend/src/metabase/components/Badge.jsx b/frontend/src/metabase/components/Badge.jsx
index c29c2f46c17..7f9ae0d0718 100644
--- a/frontend/src/metabase/components/Badge.jsx
+++ b/frontend/src/metabase/components/Badge.jsx
@@ -45,7 +45,7 @@ function Badge({
activeColor={activeColor}
{...props}
>
- {icon && <BadgeIcon {...getIconProps(icon)} hasMargin={!!children} />}
+ {icon && <BadgeIcon {...getIconProps(icon)} $hasMargin={!!children} />}
{children && <span className="text-wrap">{children}</span>}
</MaybeLink>
);
diff --git a/frontend/src/metabase/components/Badge.styled.jsx b/frontend/src/metabase/components/Badge.styled.jsx
index 292f3128e2b..7d1e9bf1ace 100644
--- a/frontend/src/metabase/components/Badge.styled.jsx
+++ b/frontend/src/metabase/components/Badge.styled.jsx
@@ -5,6 +5,7 @@ import { css } from "@emotion/react";
import { color } from "metabase/lib/colors";
import Icon from "metabase/components/Icon";
import Link from "metabase/core/components/Link";
+import { shouldForwardNonTransientProp } from "metabase/lib/styling/emotion";
const propTypes = {
to: PropTypes.oneOfType([PropTypes.string, PropTypes.bool]),
@@ -33,6 +34,8 @@ export const MaybeLink = styled(RawMaybeLink)`
}
`;
-export const BadgeIcon = styled(Icon)`
- margin-right: ${props => (props.hasMargin ? "5px" : 0)};
+export const BadgeIcon = styled(Icon, {
+ shouldForwardProp: shouldForwardNonTransientProp,
+})`
+ margin-right: ${props => (props.$hasMargin ? "5px" : 0)};
`;
diff --git a/frontend/src/metabase/components/ButtonBar.jsx b/frontend/src/metabase/components/ButtonBar.jsx
index 64ed57a4e0c..a10147b60e5 100644
--- a/frontend/src/metabase/components/ButtonBar.jsx
+++ b/frontend/src/metabase/components/ButtonBar.jsx
@@ -1,5 +1,5 @@
/* eslint-disable react/prop-types */
-import React from "react";
+import React, { Children } from "react";
import {
ButtonBarCenter,
ButtonBarLeft,
@@ -12,6 +12,8 @@ function normalizeArray(array) {
array = array.filter(a => a);
if (array.length === 0) {
array = null;
+ } else {
+ array = Children.toArray(array);
}
}
return array;
diff --git a/frontend/src/metabase/components/CollectionLanding/CollectionLanding.jsx b/frontend/src/metabase/components/CollectionLanding/CollectionLanding.jsx
index b099bfa4041..4852a23e0f6 100644
--- a/frontend/src/metabase/components/CollectionLanding/CollectionLanding.jsx
+++ b/frontend/src/metabase/components/CollectionLanding/CollectionLanding.jsx
@@ -1,11 +1,11 @@
/* eslint-disable react/prop-types */
import React, { useState } from "react";
-import * as Urls from "metabase/lib/urls";
+import { extractCollectionId } from "metabase/lib/urls";
import { PageWrapper } from "metabase/collections/components/Layout";
import CollectionContent from "metabase/collections/containers/CollectionContent";
-import CollectionSidebar from "metabase/collections/containers/CollectionSidebar/CollectionSidebar";
+import CollectionSidebar from "metabase/collections/containers/CollectionSidebar";
import { ContentBox } from "./CollectionLanding.styled";
const CollectionLanding = ({ params: { slug }, children }) => {
@@ -16,7 +16,7 @@ const CollectionLanding = ({ params: { slug }, children }) => {
const handleToggleMobileSidebar = () =>
setShouldDisplayMobileSidebar(!shouldDisplayMobileSidebar);
- const collectionId = Urls.extractCollectionId(slug);
+ const collectionId = extractCollectionId(slug);
const isRoot = collectionId === "root";
return (
diff --git a/frontend/src/metabase/components/Icon.tsx b/frontend/src/metabase/components/Icon.tsx
index 6fb2138e5cd..3e40b460db8 100644
--- a/frontend/src/metabase/components/Icon.tsx
+++ b/frontend/src/metabase/components/Icon.tsx
@@ -7,6 +7,7 @@ import Tooltip from "metabase/components/Tooltip";
import { loadIcon } from "metabase/icon_paths";
import { color as c } from "metabase/lib/colors";
import { stripLayoutProps } from "metabase/lib/utils";
+import { shouldForwardNonTransientProp } from "metabase/lib/styling/emotion";
const MISSING_ICON_NAME = "unknown";
@@ -28,7 +29,7 @@ export const IconWrapper = styled.div<IconWrapperProps>`
// special cases for certain icons
// Icon-share has a taller viewbox than most so to optically center
// the icon we need to translate it upwards
- "> .icon.icon-share": {
+ & > .icon.icon-share {
transform: translateY(-2px);
}
${hover};
@@ -115,7 +116,7 @@ class BaseIcon extends Component<IconProps> {
);
} else if (icon.svg) {
return (
- <svg
+ <StyledSVG
{...svgProps}
dangerouslySetInnerHTML={{ __html: icon.svg }}
ref={forwardedRef}
@@ -123,9 +124,9 @@ class BaseIcon extends Component<IconProps> {
);
} else if (icon.path) {
return (
- <svg {...svgProps} ref={forwardedRef}>
+ <StyledSVG {...svgProps} ref={forwardedRef}>
<path d={icon.path} />
- </svg>
+ </StyledSVG>
);
} else {
console.warn(`Icon "${name}" must have an img, svg, or path`);
@@ -134,6 +135,10 @@ class BaseIcon extends Component<IconProps> {
}
}
+const StyledSVG = styled("svg", {
+ shouldForwardProp: shouldForwardNonTransientProp,
+})``;
+
const BaseIconWithRef = forwardRef<HTMLElement, IconProps>(
function BaseIconWithRef(props, ref) {
return <BaseIcon {...props} forwardedRef={ref} />;
diff --git a/frontend/src/metabase/components/MetadataInfo/TableInfoPopover/TableInfoPopover.tsx b/frontend/src/metabase/components/MetadataInfo/TableInfoPopover/TableInfoPopover.tsx
index 91083a71880..51f6688dc1e 100644
--- a/frontend/src/metabase/components/MetadataInfo/TableInfoPopover/TableInfoPopover.tsx
+++ b/frontend/src/metabase/components/MetadataInfo/TableInfoPopover/TableInfoPopover.tsx
@@ -1,5 +1,4 @@
-import React from "react";
-import PropTypes from "prop-types";
+import React, { ReactElement } from "react";
import { hideAll } from "tippy.js";
import TippyPopover, {
@@ -8,6 +7,7 @@ import TippyPopover, {
import { isVirtualCardId } from "metabase/lib/saved-questions/saved-questions";
import { WidthBoundTableInfo } from "./TableInfoPopover.styled";
+import PropTypes from "prop-types";
export const POPOVER_DELAY: [number, number] = [500, 300];
@@ -18,7 +18,7 @@ interface TableSubset {
const propTypes = {
table: PropTypes.shape({
- id: PropTypes.oneOf([PropTypes.number, PropTypes.string]),
+ id: PropTypes.oneOfType([PropTypes.number, PropTypes.string]),
description: PropTypes.string,
}).isRequired,
children: PropTypes.node,
@@ -26,10 +26,12 @@ const propTypes = {
offset: PropTypes.arrayOf(PropTypes.number),
};
-type Props = { table: TableSubset } & Pick<
- ITippyPopoverProps,
- "children" | "placement" | "offset" | "delay"
->;
+type Props = {
+ table: TableSubset;
+ children: ReactElement;
+ placement: string;
+ offset: number[];
+} & Pick<ITippyPopoverProps, "children" | "placement" | "offset" | "delay">;
const className = "table-info-popover";
diff --git a/frontend/src/metabase/components/TokenField.jsx b/frontend/src/metabase/components/TokenField.jsx
index 8e36a715b9e..04891d52695 100644
--- a/frontend/src/metabase/components/TokenField.jsx
+++ b/frontend/src/metabase/components/TokenField.jsx
@@ -495,6 +495,7 @@ export default class TokenField extends Component {
placeholder = null;
}
+ const isControlledInput = !!this.onInputChange;
const valuesList = (
<ul
className={cx(
@@ -534,9 +535,10 @@ export default class TokenField extends Component {
// set size to be small enough that it fits in a parameter.
size={10}
placeholder={placeholder}
- value={inputValue}
+ value={isControlledInput ? inputValue : undefined}
+ defaultValue={isControlledInput ? undefined : inputValue}
onKeyDown={this.onInputKeyDown}
- onChange={this.onInputChange}
+ onChange={isControlledInput ? this.onInputChange : undefined}
onFocus={this.onInputFocus}
onBlur={this.onInputBlur}
onPaste={this.onInputPaste}
diff --git a/frontend/src/metabase/core/components/CheckBox/CheckBox.tsx b/frontend/src/metabase/core/components/CheckBox/CheckBox.tsx
index 9c05786cf93..f5913babf8e 100644
--- a/frontend/src/metabase/core/components/CheckBox/CheckBox.tsx
+++ b/frontend/src/metabase/core/components/CheckBox/CheckBox.tsx
@@ -54,15 +54,17 @@ const CheckBox = forwardRef(function Checkbox(
}: CheckBoxProps,
ref: Ref<HTMLLabelElement>,
): JSX.Element {
+ const isControlledCheckBoxInput = !!onChange;
return (
<CheckBoxRoot ref={ref} {...props}>
<CheckBoxInput
type="checkbox"
- checked={checked}
+ checked={isControlledCheckBoxInput ? !!checked : undefined}
+ defaultChecked={isControlledCheckBoxInput ? undefined : !!checked}
size={size}
disabled={disabled}
autoFocus={autoFocus}
- onChange={onChange}
+ onChange={isControlledCheckBoxInput ? onChange : undefined}
onFocus={onFocus}
onBlur={onBlur}
/>
diff --git a/frontend/src/metabase/core/components/Link/Link.styled.tsx b/frontend/src/metabase/core/components/Link/Link.styled.tsx
index 7e681006d4e..5c468a87af5 100644
--- a/frontend/src/metabase/core/components/Link/Link.styled.tsx
+++ b/frontend/src/metabase/core/components/Link/Link.styled.tsx
@@ -2,8 +2,11 @@ import styled from "@emotion/styled";
import { display, space, hover, color } from "styled-system";
import { Link } from "react-router";
import { color as colors } from "metabase/lib/colors";
+import { shouldForwardNonTransientProp } from "metabase/lib/styling/emotion";
-export const LinkRoot = styled(Link)`
+export const LinkRoot = styled(Link, {
+ shouldForwardProp: shouldForwardNonTransientProp,
+})`
${display};
${space};
${hover};
diff --git a/frontend/src/metabase/lib/styling/emotion.ts b/frontend/src/metabase/lib/styling/emotion.ts
new file mode 100644
index 00000000000..9fd3588ba7b
--- /dev/null
+++ b/frontend/src/metabase/lib/styling/emotion.ts
@@ -0,0 +1,5 @@
+import isPropValid from "@emotion/is-prop-valid";
+
+export function shouldForwardNonTransientProp(propName: string): boolean {
+ return !propName.startsWith("$") && isPropValid(propName);
+}
diff --git a/frontend/src/metabase/parameters/components/ParameterValueWidget.jsx b/frontend/src/metabase/parameters/components/ParameterValueWidget.jsx
index bb84372d66e..b60831c896e 100644
--- a/frontend/src/metabase/parameters/components/ParameterValueWidget.jsx
+++ b/frontend/src/metabase/parameters/components/ParameterValueWidget.jsx
@@ -181,7 +181,7 @@ export default class ParameterValueWidget extends Component {
isFullscreen={isFullscreen}
hasValue={hasValue}
noReset={noReset}
- noPopover={noPopover}
+ noPopover={!!noPopover}
isFocused={isFocused}
setValue={setValue}
/>
@@ -216,7 +216,7 @@ export default class ParameterValueWidget extends Component {
isFullscreen={isFullscreen}
hasValue={hasValue}
noReset={noReset}
- noPopover={noPopover}
+ noPopover={!!noPopover}
isFocused={isFocused}
setValue={setValue}
/>
diff --git a/frontend/src/metabase/query_builder/components/view/sidebars/SummarizeSidebar/AddAggregationButton/AddAggregationButton.jsx b/frontend/src/metabase/query_builder/components/view/sidebars/SummarizeSidebar/AddAggregationButton/AddAggregationButton.jsx
index 9b3e36078c9..46ca1b6fbd6 100644
--- a/frontend/src/metabase/query_builder/components/view/sidebars/SummarizeSidebar/AddAggregationButton/AddAggregationButton.jsx
+++ b/frontend/src/metabase/query_builder/components/view/sidebars/SummarizeSidebar/AddAggregationButton/AddAggregationButton.jsx
@@ -17,7 +17,7 @@ const propTypes = {
const LABEL = t`Add a metric`;
-export const AddAggregationButton = ({ query, shouldShowLabel }) => {
+export const AddAggregationButton = ({ query, shouldShowLabel = false }) => {
return (
<PopoverWithTrigger
triggerElement={
diff --git a/frontend/src/metabase/questions/components/CollectionBadge.jsx b/frontend/src/metabase/questions/components/CollectionBadge.jsx
index 9576363e161..39d7c3cef6a 100644
--- a/frontend/src/metabase/questions/components/CollectionBadge.jsx
+++ b/frontend/src/metabase/questions/components/CollectionBadge.jsx
@@ -8,7 +8,7 @@ import { PLUGIN_COLLECTIONS } from "metabase/plugins";
const propTypes = {
collection: PropTypes.object,
- analyticsContext: PropTypes.string.isRequired,
+ analyticsContext: PropTypes.string,
className: PropTypes.string,
};
@@ -25,6 +25,7 @@ function CollectionBadge({ collection, analyticsContext, className }) {
if (!collection) {
return null;
}
+
const isRegular = PLUGIN_COLLECTIONS.isRegularCollection(collection);
const icon = {
...collection.getIcon(),
--
GitLab