From 2f336c0c16d0b99a6ecfc42e0a286228988a8443 Mon Sep 17 00:00:00 2001
From: rtexelm <texelist@gmail.com>
Date: Fri, 17 May 2024 09:14:04 -0400
Subject: [PATCH 1/3] Add null return, refactor tests

---
 .../src/chart-composition/ChartFrame.tsx      |   8 +-
 .../chart-composition/ChartFrame.test.tsx     | 175 +++++++++---------
 2 files changed, 96 insertions(+), 87 deletions(-)

diff --git a/superset-frontend/packages/superset-ui-core/src/chart-composition/ChartFrame.tsx b/superset-frontend/packages/superset-ui-core/src/chart-composition/ChartFrame.tsx
index 784097b4b6b96..385cae6ffde2c 100644
--- a/superset-frontend/packages/superset-ui-core/src/chart-composition/ChartFrame.tsx
+++ b/superset-frontend/packages/superset-ui-core/src/chart-composition/ChartFrame.tsx
@@ -28,7 +28,7 @@ type Props = {
   contentWidth?: number;
   contentHeight?: number;
   height: number;
-  renderContent: ({
+  renderContent?: ({
     height,
     width,
   }: {
@@ -40,13 +40,17 @@ type Props = {
 
 export default class ChartFrame extends PureComponent<Props, {}> {
   static defaultProps = {
-    renderContent() {},
+    renderContent: () => null,
   };
 
   render() {
     const { contentWidth, contentHeight, width, height, renderContent } =
       this.props;
 
+    if (!renderContent) {
+      return null;
+    }
+
     const overflowX = checkNumber(contentWidth) && contentWidth > width;
     const overflowY = checkNumber(contentHeight) && contentHeight > height;
 
diff --git a/superset-frontend/packages/superset-ui-core/test/chart-composition/ChartFrame.test.tsx b/superset-frontend/packages/superset-ui-core/test/chart-composition/ChartFrame.test.tsx
index 777a7b2a2a521..b6351a90b4fb1 100644
--- a/superset-frontend/packages/superset-ui-core/test/chart-composition/ChartFrame.test.tsx
+++ b/superset-frontend/packages/superset-ui-core/test/chart-composition/ChartFrame.test.tsx
@@ -18,110 +18,115 @@
  */
 
 import React from 'react';
-import { shallow } from 'enzyme';
+import { render } from '@testing-library/react';
+import '@testing-library/jest-dom';
 import { ChartFrame } from '@superset-ui/core';
 
-describe('TooltipFrame', () => {
+type Props = {
+  contentWidth?: number;
+  contentHeight?: number;
+  height: number;
+  renderContent?: ({
+    height,
+    width,
+  }: {
+    height: number;
+    width: number;
+  }) => React.ReactNode;
+  width: number;
+};
+
+const renderChartFrame = (props: Props) => render(<ChartFrame {...props} />);
+
+describe('ChartFrame', () => {
   it('renders content that requires smaller space than frame', () => {
-    const wrapper = shallow(
-      <ChartFrame
-        width={400}
-        height={400}
-        contentWidth={300}
-        contentHeight={300}
-        renderContent={({ width, height }) => (
-          <div>
-            {width}/{height}
-          </div>
-        )}
-      />,
-    );
-    expect(wrapper.find('div').text()).toEqual('400/400');
+    const { getByText } = renderChartFrame({
+      width: 400,
+      height: 400,
+      contentWidth: 300,
+      contentHeight: 300,
+      renderContent: ({ width, height }) => (
+        <div>
+          {width}/{height}
+        </div>
+      ),
+    });
+    expect(getByText('400/400')).toBeInTheDocument();
   });
 
   it('renders content without specifying content size', () => {
-    const wrapper = shallow(
-      <ChartFrame
-        width={400}
-        height={400}
-        renderContent={({ width, height }) => (
-          <div>
-            {width}/{height}
-          </div>
-        )}
-      />,
-    );
-    expect(wrapper.find('div').text()).toEqual('400/400');
+    const { getByText } = renderChartFrame({
+      width: 400,
+      height: 400,
+      renderContent: ({ width, height }) => (
+        <div>
+          {width}/{height}
+        </div>
+      ),
+    });
+    expect(getByText('400/400')).toBeInTheDocument();
   });
 
   it('renders content that requires same size with frame', () => {
-    const wrapper = shallow(
-      <ChartFrame
-        width={400}
-        height={400}
-        contentWidth={400}
-        contentHeight={400}
-        renderContent={({ width, height }) => (
-          <div>
-            {width}/{height}
-          </div>
-        )}
-      />,
-    );
-    expect(wrapper.find('div').text()).toEqual('400/400');
+    const { getByText } = renderChartFrame({
+      width: 400,
+      height: 400,
+      contentWidth: 400,
+      contentHeight: 400,
+      renderContent: ({ width, height }) => (
+        <div>
+          {width}/{height}
+        </div>
+      ),
+    });
+    expect(getByText('400/400')).toBeInTheDocument();
   });
 
   it('renders content that requires space larger than frame', () => {
-    const wrapper = shallow(
-      <ChartFrame
-        width={400}
-        height={400}
-        contentWidth={500}
-        contentHeight={500}
-        renderContent={({ width, height }) => (
-          <div className="chart">
-            {width}/{height}
-          </div>
-        )}
-      />,
-    );
-    expect(wrapper.find('div.chart').text()).toEqual('500/500');
+    const { getByText } = renderChartFrame({
+      width: 400,
+      height: 400,
+      contentWidth: 500,
+      contentHeight: 500,
+      renderContent: ({ width, height }) => (
+        <div className="chart">
+          {width}/{height}
+        </div>
+      ),
+    });
+    expect(getByText('500/500')).toBeInTheDocument();
   });
 
   it('renders content that width is larger than frame', () => {
-    const wrapper = shallow(
-      <ChartFrame
-        width={400}
-        height={400}
-        contentWidth={500}
-        renderContent={({ width, height }) => (
-          <div className="chart">
-            {width}/{height}
-          </div>
-        )}
-      />,
-    );
-    expect(wrapper.find('div.chart').text()).toEqual('500/400');
+    const { getByText } = renderChartFrame({
+      width: 400,
+      height: 400,
+      contentWidth: 500,
+      renderContent: ({ width, height }) => (
+        <div className="chart">
+          {width}/{height}
+        </div>
+      ),
+    });
+    expect(getByText('500/400')).toBeInTheDocument();
   });
 
   it('renders content that height is larger than frame', () => {
-    const wrapper = shallow(
-      <ChartFrame
-        width={400}
-        height={400}
-        contentHeight={600}
-        renderContent={({ width, height }) => (
-          <div className="chart">
-            {width}/{height}
-          </div>
-        )}
-      />,
-    );
-    expect(wrapper.find('div.chart').text()).toEqual('400/600');
+    const { getByText } = renderChartFrame({
+      width: 400,
+      height: 400,
+      contentHeight: 600,
+      renderContent: ({ width, height }) => (
+        <div className="chart">
+          {width}/{height}
+        </div>
+      ),
+    });
+    expect(getByText('400/600')).toBeInTheDocument();
   });
 
-  it('renders an empty frame when renderContent is not given', () => {
-    const wrapper = shallow(<ChartFrame width={400} height={400} />);
-    expect(wrapper.find('div')).toHaveLength(0);
+  it('renders an empty container if renderContent is not provided', () => {
+    const { container } = render(<ChartFrame width={400} height={300} />);
+    expect(container).toBeEmptyDOMElement();
   });
 });

From c481fb8332df2f3250a269a567ebcf1f348c93b8 Mon Sep 17 00:00:00 2001
From: rtexelm <texelist@gmail.com>
Date: Fri, 17 May 2024 12:53:15 -0400
Subject: [PATCH 2/3] Remove describe block

---
 .../chart-composition/ChartFrame.test.tsx     | 156 +++++++++---------
 1 file changed, 77 insertions(+), 79 deletions(-)

diff --git a/superset-frontend/packages/superset-ui-core/test/chart-composition/ChartFrame.test.tsx b/superset-frontend/packages/superset-ui-core/test/chart-composition/ChartFrame.test.tsx
index b6351a90b4fb1..b7e531e4ac610 100644
--- a/superset-frontend/packages/superset-ui-core/test/chart-composition/ChartFrame.test.tsx
+++ b/superset-frontend/packages/superset-ui-core/test/chart-composition/ChartFrame.test.tsx
@@ -38,95 +38,93 @@ type Props = {
 
 const renderChartFrame = (props: Props) => render(<ChartFrame {...props} />);
 
-describe('ChartFrame', () => {
-  it('renders content that requires smaller space than frame', () => {
-    const { getByText } = renderChartFrame({
-      width: 400,
-      height: 400,
-      contentWidth: 300,
-      contentHeight: 300,
-      renderContent: ({ width, height }) => (
-        <div>
-          {width}/{height}
-        </div>
-      ),
-    });
-    expect(getByText('400/400')).toBeInTheDocument();
+it('renders content that requires smaller space than frame', () => {
+  const { getByText } = renderChartFrame({
+    width: 400,
+    height: 400,
+    contentWidth: 300,
+    contentHeight: 300,
+    renderContent: ({ width, height }) => (
+      <div>
+        {width}/{height}
+      </div>
+    ),
   });
+  expect(getByText('400/400')).toBeInTheDocument();
+});
 
-  it('renders content without specifying content size', () => {
-    const { getByText } = renderChartFrame({
-      width: 400,
-      height: 400,
-      renderContent: ({ width, height }) => (
-        <div>
-          {width}/{height}
-        </div>
-      ),
-    });
-    expect(getByText('400/400')).toBeInTheDocument();
+it('renders content without specifying content size', () => {
+  const { getByText } = renderChartFrame({
+    width: 400,
+    height: 400,
+    renderContent: ({ width, height }) => (
+      <div>
+        {width}/{height}
+      </div>
+    ),
   });
+  expect(getByText('400/400')).toBeInTheDocument();
+});
 
-  it('renders content that requires same size with frame', () => {
-    const { getByText } = renderChartFrame({
-      width: 400,
-      height: 400,
-      contentWidth: 400,
-      contentHeight: 400,
-      renderContent: ({ width, height }) => (
-        <div>
-          {width}/{height}
-        </div>
-      ),
-    });
-    expect(getByText('400/400')).toBeInTheDocument();
+it('renders content that requires same size with frame', () => {
+  const { getByText } = renderChartFrame({
+    width: 400,
+    height: 400,
+    contentWidth: 400,
+    contentHeight: 400,
+    renderContent: ({ width, height }) => (
+      <div>
+        {width}/{height}
+      </div>
+    ),
   });
+  expect(getByText('400/400')).toBeInTheDocument();
+});
 
-  it('renders content that requires space larger than frame', () => {
-    const { getByText } = renderChartFrame({
-      width: 400,
-      height: 400,
-      contentWidth: 500,
-      contentHeight: 500,
-      renderContent: ({ width, height }) => (
-        <div className="chart">
-          {width}/{height}
-        </div>
-      ),
-    });
-    expect(getByText('500/500')).toBeInTheDocument();
+it('renders content that requires space larger than frame', () => {
+  const { getByText } = renderChartFrame({
+    width: 400,
+    height: 400,
+    contentWidth: 500,
+    contentHeight: 500,
+    renderContent: ({ width, height }) => (
+      <div className="chart">
+        {width}/{height}
+      </div>
+    ),
   });
+  expect(getByText('500/500')).toBeInTheDocument();
+});
 
-  it('renders content that width is larger than frame', () => {
-    const { getByText } = renderChartFrame({
-      width: 400,
-      height: 400,
-      contentWidth: 500,
-      renderContent: ({ width, height }) => (
-        <div className="chart">
-          {width}/{height}
-        </div>
-      ),
-    });
-    expect(getByText('500/400')).toBeInTheDocument();
+it('renders content that width is larger than frame', () => {
+  const { getByText } = renderChartFrame({
+    width: 400,
+    height: 400,
+    contentWidth: 500,
+    renderContent: ({ width, height }) => (
+      <div className="chart">
+        {width}/{height}
+      </div>
+    ),
   });
+  expect(getByText('500/400')).toBeInTheDocument();
+});
 
-  it('renders content that height is larger than frame', () => {
-    const { getByText } = renderChartFrame({
-      width: 400,
-      height: 400,
-      contentHeight: 600,
-      renderContent: ({ width, height }) => (
-        <div className="chart">
-          {width}/{height}
-        </div>
-      ),
-    });
-    expect(getByText('400/600')).toBeInTheDocument();
+it('renders content that height is larger than frame', () => {
+  const { getByText } = renderChartFrame({
+    width: 400,
+    height: 400,
+    contentHeight: 600,
+    renderContent: ({ width, height }) => (
+      <div className="chart">
+        {width}/{height}
+      </div>
+    ),
   });
+  expect(getByText('400/600')).toBeInTheDocument();
+});
 
-  it('renders an empty container if renderContent is not provided', () => {
-    const { container } = render(<ChartFrame width={400} height={300} />);
-    expect(container).toBeEmptyDOMElement();
-  });
+it('renders an empty container if renderContent is not provided', () => {
+  const { container } = render(<ChartFrame width={400} height={300} />);
+  expect(container).toBeEmptyDOMElement();
 });

From 5110165efed429855ae4355f5e5ea0bd325cf2b5 Mon Sep 17 00:00:00 2001
From: rtexelm <texelist@gmail.com>
Date: Mon, 20 May 2024 16:35:38 -0400
Subject: [PATCH 3/3] Re-refactor tests, omit unnecsry, add new

---
 .../src/chart-composition/ChartFrame.tsx      |  8 ++----
 .../chart-composition/ChartFrame.test.tsx     | 25 ++++++++++---------
 2 files changed, 15 insertions(+), 18 deletions(-)

diff --git a/superset-frontend/packages/superset-ui-core/src/chart-composition/ChartFrame.tsx b/superset-frontend/packages/superset-ui-core/src/chart-composition/ChartFrame.tsx
index 385cae6ffde2c..784097b4b6b96 100644
--- a/superset-frontend/packages/superset-ui-core/src/chart-composition/ChartFrame.tsx
+++ b/superset-frontend/packages/superset-ui-core/src/chart-composition/ChartFrame.tsx
@@ -28,7 +28,7 @@ type Props = {
   contentWidth?: number;
   contentHeight?: number;
   height: number;
-  renderContent?: ({
+  renderContent: ({
     height,
     width,
   }: {
@@ -40,17 +40,13 @@ type Props = {
 
 export default class ChartFrame extends PureComponent<Props, {}> {
   static defaultProps = {
-    renderContent: () => null,
+    renderContent() {},
   };
 
   render() {
     const { contentWidth, contentHeight, width, height, renderContent } =
       this.props;
 
-    if (!renderContent) {
-      return null;
-    }
-
     const overflowX = checkNumber(contentWidth) && contentWidth > width;
     const overflowY = checkNumber(contentHeight) && contentHeight > height;
 
diff --git a/superset-frontend/packages/superset-ui-core/test/chart-composition/ChartFrame.test.tsx b/superset-frontend/packages/superset-ui-core/test/chart-composition/ChartFrame.test.tsx
index b7e531e4ac610..a2573c4208b63 100644
--- a/superset-frontend/packages/superset-ui-core/test/chart-composition/ChartFrame.test.tsx
+++ b/superset-frontend/packages/superset-ui-core/test/chart-composition/ChartFrame.test.tsx
@@ -26,7 +26,7 @@ type Props = {
   contentWidth?: number;
   contentHeight?: number;
   height: number;
-  renderContent?: ({
+  renderContent: ({
     height,
     width,
   }: {
@@ -66,7 +66,7 @@ it('renders content without specifying content size', () => {
   expect(getByText('400/400')).toBeInTheDocument();
 });
 
-it('renders content that requires same size with frame', () => {
+it('renders content that requires equivalent size to frame', () => {
   const { getByText } = renderChartFrame({
     width: 400,
     height: 400,
@@ -82,7 +82,7 @@ it('renders content that requires same size with frame', () => {
 });
 
 it('renders content that requires space larger than frame', () => {
-  const { getByText } = renderChartFrame({
+  const { getByText, container } = renderChartFrame({
     width: 400,
     height: 400,
     contentWidth: 500,
@@ -94,10 +94,12 @@ it('renders content that requires space larger than frame', () => {
     ),
   });
   expect(getByText('500/500')).toBeInTheDocument();
+  const containerDiv = container.firstChild as HTMLElement;
+  expect(containerDiv).toHaveStyle({ overflowX: 'auto', overflowY: 'auto' });
 });
 
-it('renders content that width is larger than frame', () => {
-  const { getByText } = renderChartFrame({
+it('renders content when width is larger than frame', () => {
+  const { getByText, container } = renderChartFrame({
     width: 400,
     height: 400,
     contentWidth: 500,
@@ -108,10 +110,12 @@ it('renders content that width is larger than frame', () => {
     ),
   });
   expect(getByText('500/400')).toBeInTheDocument();
+  const containerDiv = container.firstChild as HTMLElement;
+  expect(containerDiv).toHaveStyle({ overflowX: 'auto', overflowY: 'hidden' });
 });
 
-it('renders content that height is larger than frame', () => {
-  const { getByText } = renderChartFrame({
+it('renders content when height is larger than frame', () => {
+  const { getByText, container } = renderChartFrame({
     width: 400,
     height: 400,
     contentHeight: 600,
@@ -122,9 +126,6 @@ it('renders content that height is larger than frame', () => {
     ),
   });
   expect(getByText('400/600')).toBeInTheDocument();
-});
-
-it('renders an empty container if renderContent is not provided', () => {
-  const { container } = render(<ChartFrame width={400} height={300} />);
-  expect(container).toBeEmptyDOMElement();
+  const containerDiv = container.firstChild as HTMLElement;
+  expect(containerDiv).toHaveStyle({ overflowX: 'hidden', overflowY: 'auto' });
 });