From c8aa0a5420d8780ce347cecb58c51aaea57d26ad Mon Sep 17 00:00:00 2001
From: Romeo Van Snick <romeo@romeovansnick.be>
Date: Mon, 18 Nov 2024 21:54:42 +1100
Subject: [PATCH] Fix button group breaking (#50064)
* Use Flex instead of Group for items in button
* Allow column picker to stretch to match the height of the element
* Fix height of data picker buttons
* Make sure the whole button is clickable
* Add e2e test for metabase/metabase#50038
* Use correct id for issue in e2e test
Co-authored-by: Kamil Mielnik <kamil@kamilmielnik.com>
---------
Co-authored-by: Kamil Mielnik <kamil@kamilmielnik.com>
---
.../reproductions-3.cy.spec.js | 79 +++++++++++++++++++
.../components/DataStep/DataStep.styled.tsx | 1 +
.../notebook/components/DataStep/DataStep.tsx | 2 +-
.../JoinTablePicker.styled.tsx | 1 +
.../JoinTablePicker/JoinTablePicker.tsx | 2 +-
.../NotebookCell/NotebookCell.styled.tsx | 2 +-
.../NotebookDataPicker/NotebookDataPicker.tsx | 10 ++-
7 files changed, 90 insertions(+), 7 deletions(-)
diff --git a/e2e/test/scenarios/question-reproductions/reproductions-3.cy.spec.js b/e2e/test/scenarios/question-reproductions/reproductions-3.cy.spec.js
index 4bec179325b..025ee172378 100644
--- a/e2e/test/scenarios/question-reproductions/reproductions-3.cy.spec.js
+++ b/e2e/test/scenarios/question-reproductions/reproductions-3.cy.spec.js
@@ -2357,3 +2357,82 @@ describe("issue 48829", () => {
modal().should("not.exist");
});
});
+
+describe("issue 50038", () => {
+ const QUESTION = {
+ name: "question with a very long name that will be too long to fit on one line which normally would result in some weird looking buttons with inconsistent heights",
+ query: {
+ "source-table": PRODUCTS_ID,
+ },
+ };
+
+ const OTHER_QUESTION = {
+ name: "question that also has a long name that is so long it will break in the button",
+ query: {
+ "source-table": ORDERS_ID,
+ },
+ };
+
+ beforeEach(() => {
+ restore();
+ cy.signInAsNormalUser();
+
+ createQuestion(QUESTION, { wrapId: true, idAlias: "questionId" });
+ createQuestion(OTHER_QUESTION, {
+ wrapId: true,
+ idAlias: "otherQuestionId",
+ });
+
+ cy.get("@questionId").then(questionId => {
+ cy.get("@otherQuestionId").then(otherQuestionId => {
+ createQuestion(
+ {
+ name: "Joined question",
+ query: {
+ "source-table": `card__${questionId}`,
+ joins: [
+ {
+ "source-table": `card__${otherQuestionId}`,
+ fields: "all",
+ strategy: "left-join",
+ condition: [
+ "=",
+ ["field", ORDERS_ID, {}],
+ ["field", PRODUCTS_ID, {}],
+ ],
+ },
+ ],
+ },
+ },
+ { visitQuestion: true },
+ );
+ });
+ });
+ });
+
+ function assertEqualHeight(selector, otherSelector) {
+ selector.invoke("outerHeight").then(height => {
+ otherSelector.invoke("outerHeight").should("eq", height);
+ });
+ }
+
+ it("should not break data source and join source buttons when the source names are too long (metabase#50038)", () => {
+ openNotebook();
+ getNotebookStep("data").within(() => {
+ assertEqualHeight(
+ cy.findByText(QUESTION.name).parent().should("be.visible"),
+ cy.findByTestId("fields-picker").should("be.visible"),
+ );
+ });
+ getNotebookStep("join").within(() => {
+ assertEqualHeight(
+ cy
+ .findAllByText(OTHER_QUESTION.name)
+ .first()
+ .parent()
+ .should("be.visible"),
+ cy.findByTestId("fields-picker").should("be.visible"),
+ );
+ });
+ });
+});
diff --git a/frontend/src/metabase/querying/notebook/components/DataStep/DataStep.styled.tsx b/frontend/src/metabase/querying/notebook/components/DataStep/DataStep.styled.tsx
index ad6a721e491..dd41a5931ad 100644
--- a/frontend/src/metabase/querying/notebook/components/DataStep/DataStep.styled.tsx
+++ b/frontend/src/metabase/querying/notebook/components/DataStep/DataStep.styled.tsx
@@ -8,4 +8,5 @@ export const DataStepIconButton = styled(IconButtonWrapper)`
color: var(--mb-color-text-white);
padding: ${NotebookCell.CONTAINER_PADDING};
opacity: 0.5;
+ height: 100%;
`;
diff --git a/frontend/src/metabase/querying/notebook/components/DataStep/DataStep.tsx b/frontend/src/metabase/querying/notebook/components/DataStep/DataStep.tsx
index fb0bd60ec91..344f6d39963 100644
--- a/frontend/src/metabase/querying/notebook/components/DataStep/DataStep.tsx
+++ b/frontend/src/metabase/querying/notebook/components/DataStep/DataStep.tsx
@@ -60,7 +60,7 @@ export const DataStep = ({
)
}
containerStyle={{ padding: 0 }}
- rightContainerStyle={{ width: 37, height: 37, padding: 0 }}
+ rightContainerStyle={{ width: 37, padding: 0 }}
data-testid="data-step-cell"
>
<NotebookDataPicker
diff --git a/frontend/src/metabase/querying/notebook/components/JoinStep/JoinTablePicker/JoinTablePicker.styled.tsx b/frontend/src/metabase/querying/notebook/components/JoinStep/JoinTablePicker/JoinTablePicker.styled.tsx
index 6e6675a8fce..1d38b2b47d1 100644
--- a/frontend/src/metabase/querying/notebook/components/JoinStep/JoinTablePicker/JoinTablePicker.styled.tsx
+++ b/frontend/src/metabase/querying/notebook/components/JoinStep/JoinTablePicker/JoinTablePicker.styled.tsx
@@ -8,4 +8,5 @@ export const ColumnPickerButton = styled(IconButtonWrapper)`
padding: ${NotebookCell.CONTAINER_PADDING};
opacity: 0.5;
color: var(--mb-color-text-white);
+ height: 100%;
`;
diff --git a/frontend/src/metabase/querying/notebook/components/JoinStep/JoinTablePicker/JoinTablePicker.tsx b/frontend/src/metabase/querying/notebook/components/JoinStep/JoinTablePicker/JoinTablePicker.tsx
index e8a79745ba4..689bb8d226e 100644
--- a/frontend/src/metabase/querying/notebook/components/JoinStep/JoinTablePicker/JoinTablePicker.tsx
+++ b/frontend/src/metabase/querying/notebook/components/JoinStep/JoinTablePicker/JoinTablePicker.tsx
@@ -92,6 +92,6 @@ const CONTAINER_STYLE = {
const RIGHT_CONTAINER_STYLE = {
width: 37,
- height: 37,
+ height: "100%",
padding: 0,
};
diff --git a/frontend/src/metabase/querying/notebook/components/NotebookCell/NotebookCell.styled.tsx b/frontend/src/metabase/querying/notebook/components/NotebookCell/NotebookCell.styled.tsx
index 11ff972bcba..75c743bc0c6 100644
--- a/frontend/src/metabase/querying/notebook/components/NotebookCell/NotebookCell.styled.tsx
+++ b/frontend/src/metabase/querying/notebook/components/NotebookCell/NotebookCell.styled.tsx
@@ -24,7 +24,6 @@ export const NotebookCellItemContainer = styled.div<{
disabled?: boolean;
}>`
display: flex;
- align-items: center;
font-weight: bold;
color: ${props => (props.inactive ? props.color : color("text-white"))};
border-radius: 6px;
@@ -36,6 +35,7 @@ export const NotebookCellItemContainer = styled.div<{
? "pointer"
: "default"};
pointer-events: ${props => (props.disabled ? "none" : "auto")};
+ align-items: stretch;
&:hover {
border-color: ${props => props.inactive && alpha(props.color, 0.8)};
diff --git a/frontend/src/metabase/querying/notebook/components/NotebookDataPicker/NotebookDataPicker.tsx b/frontend/src/metabase/querying/notebook/components/NotebookDataPicker/NotebookDataPicker.tsx
index 8500a3980dd..fbf6978a0f2 100644
--- a/frontend/src/metabase/querying/notebook/components/NotebookDataPicker/NotebookDataPicker.tsx
+++ b/frontend/src/metabase/querying/notebook/components/NotebookDataPicker/NotebookDataPicker.tsx
@@ -14,7 +14,7 @@ import { loadMetadataForTable } from "metabase/questions/actions";
import { getIsEmbeddingSdk } from "metabase/selectors/embed";
import { getMetadata } from "metabase/selectors/metadata";
import type { IconName } from "metabase/ui";
-import { Group, Icon, Tooltip, UnstyledButton } from "metabase/ui";
+import { Flex, Icon, Tooltip, UnstyledButton } from "metabase/ui";
import * as Lib from "metabase-lib";
import type { DatabaseId, TableId } from "metabase-types/api";
@@ -128,10 +128,12 @@ export function NotebookDataPicker({
onClick={handleClick}
onAuxClick={handleAuxClick}
>
- <Group spacing="xs">
- {tableInfo && <Icon name={getTableIcon(tableInfo)} />}
+ <Flex align="center" gap="xs">
+ {tableInfo && (
+ <Icon name={getTableIcon(tableInfo)} style={{ flexShrink: 0 }} />
+ )}
{tableInfo?.displayName ?? placeholder}
- </Group>
+ </Flex>
</UnstyledButton>
</Tooltip>
{isOpen && (
--
GitLab