aminghadersohi commented on code in PR #39423:
URL: https://github.com/apache/superset/pull/39423#discussion_r3191995886
##########
superset-frontend/src/dashboard/components/DashboardBuilder/DashboardBuilder.test.tsx:
##########
@@ -487,6 +487,42 @@ test('should render ParentSize wrapper with height 100%
for tabs', async () => {
expect(tabPanels.length).toBeGreaterThan(0);
});
+test('should apply min-height to the top-level tab drop target so tabs can be
dropped on dashboards with content', () => {
+ (useStoredSidebarWidth as jest.Mock).mockImplementation(() => [
+ 100,
+ jest.fn(),
+ ]);
+ (fetchFaveStar as jest.Mock).mockReturnValue({ type: 'mock-action' });
+ (setActiveTab as jest.Mock).mockReturnValue({ type: 'mock-action' });
+
+ const { getByTestId } = render(<DashboardBuilder />, {
+ useRedux: true,
+ store: storeWithState({
+ ...mockState,
+ dashboardLayout: undoableDashboardLayout,
+ dashboardState: { ...mockState.dashboardState, editMode: true },
+ }),
+ useDnd: true,
+ useTheme: true,
+ });
+
+ const headerWrapper = getByTestId('dashboard-header-wrapper');
+
+ // The Droppable inside the header should have the empty-droptarget class
+ // when there are no top-level tabs and edit mode is active. Without this
+ // class (and its associated min-height CSS rule), the drop target has zero
+ // height and users cannot drag tabs onto dashboards that already have
+ // content.
+ const droptarget = headerWrapper.querySelector('.empty-droptarget');
+ expect(droptarget).toBeInTheDocument();
+
+ // Verify the StyledHeader CSS defines a non-zero min-height for
+ // .empty-droptarget (theme.sizeUnit * 4 = 16px with the default theme).
+ expect(headerWrapper).toHaveStyleRule('min-height', '16px', {
Review Comment:
Nit: hard-codes `'16px'` which implicitly assumes `theme.sizeUnit === 4`. If
that token ever changes, this test fails with a confusing mismatch rather than
a meaningful regression signal. Consider computing from the theme:
```tsx
import { supersetTheme } from '@superset-ui/core';
// ...
expect(headerWrapper).toHaveStyleRule(
'min-height',
`${supersetTheme.sizeUnit * 4}px`,
{ target: '.empty-droptarget' },
);
```
##########
superset-frontend/src/types/emotion-jest.d.ts:
##########
@@ -0,0 +1,20 @@
+/**
+ * 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.
+ */
+
+/// <reference types="@emotion/jest" />
Review Comment:
Nit: this triple-slash directive augments `expect` globally across all
`src/` code, not just tests. Harmless in practice since it only adds a type,
but the augmentation could live more precisely in `spec/helpers/setup.ts`
(which already does `import { matchers } from '@emotion/jest'`) or via a
test-only tsconfig `types` entry. Not a blocker — putting it in `src/types/` is
consistent with the existing project convention for ambient declarations.
--
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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
