geido commented on code in PR #33809:
URL: https://github.com/apache/superset/pull/33809#discussion_r2154298638
##########
superset-frontend/src/dashboard/components/Header/Header.test.tsx:
##########
@@ -484,3 +495,99 @@ test('should render MetadataBar when not in edit mode and
not embedded', () => {
screen.getByText(state.dashboardInfo.changed_on_delta_humanized),
).toBeInTheDocument();
});
+
+test('should show UnsavedChangesModal when there are unsaved changes and user
tries to navigate', async () => {
+ (useUnsavedChangesPrompt as jest.Mock).mockReturnValue({
+ showModal: true,
+ setShowModal: jest.fn(),
+ handleConfirmNavigation: jest.fn(),
+ handleSaveAndCloseModal: jest.fn(),
+ });
+
+ setup({ ...editableState });
+
+ await waitFor(() => {
+ expect(
+ screen.getByText('Save changes to your dashboard?'),
+ ).toBeInTheDocument();
+
+ expect(
+ screen.getByText("If you don't save, changes will be lost."),
+ ).toBeInTheDocument();
+ });
+});
+test('should call handleSaveAndCloseModal when Save is clicked in
UnsavedChangesModal', async () => {
+ const handleSaveAndCloseModal = jest.fn();
+
+ (useUnsavedChangesPrompt as jest.Mock).mockReturnValue({
+ showModal: true,
+ setShowModal: jest.fn(),
+ handleConfirmNavigation: jest.fn(),
+ handleSaveAndCloseModal,
+ });
+
+ setup({ ...editableState });
+
+ await waitFor(() => {
Review Comment:
Should we use `findBy` instead of `waitFor`?
##########
superset-frontend/src/components/UnsavedChangesModal/UnsavedChangesModal.test.tsx:
##########
@@ -0,0 +1,106 @@
+/**
+ * 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.
+ */
+import { fireEvent, render, waitFor } from 'spec/helpers/testing-library';
+import UnsavedChangesModal from '.';
+
+test('should render nothing if showModal is false', () => {
+ const { queryByTestId } = render(
+ <UnsavedChangesModal
+ showModal={false}
+ onHide={() => {}}
+ handleSave={() => {}}
+ onConfirmNavigation={() => {}}
+ />,
+ );
+
+ expect(queryByTestId('unsaved-changes-modal')).not.toBeInTheDocument();
+});
+
+test('should render the UnsavedChangesModal component if showModal is true',
async () => {
+ const { findByTestId } = render(
+ <UnsavedChangesModal
+ showModal
+ onHide={() => {}}
+ handleSave={() => {}}
+ onConfirmNavigation={() => {}}
+ />,
+ );
+
+ expect(await findByTestId('unsaved-changes-modal')).toBeInTheDocument();
+});
+
+test('should only call onConfirmNavigation when click on the modal Discard
button', async () => {
+ const mockOnHide = jest.fn();
+ const mockHandleSave = jest.fn();
+ const mockOnConfirmNavigation = jest.fn();
+
+ const { findByTestId } = render(
+ <UnsavedChangesModal
+ showModal
+ onHide={mockOnHide}
+ handleSave={mockHandleSave}
+ onConfirmNavigation={mockOnConfirmNavigation}
+ />,
+ );
+
+ await waitFor(async () => {
+ expect(mockOnHide).toHaveBeenCalledTimes(0);
+ expect(mockHandleSave).toHaveBeenCalledTimes(0);
+ expect(mockOnConfirmNavigation).toHaveBeenCalledTimes(0);
+
+ const cancelButton: HTMLElement = await findByTestId(
+ 'unsaved-modal-discard-button',
+ );
+ fireEvent.click(cancelButton);
Review Comment:
`userEvent` could be preferrable
##########
superset-frontend/src/components/UnsavedChangesModal/UnsavedChangesModal.test.tsx:
##########
@@ -0,0 +1,106 @@
+/**
+ * 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.
+ */
+import { fireEvent, render, waitFor } from 'spec/helpers/testing-library';
+import UnsavedChangesModal from '.';
+
+test('should render nothing if showModal is false', () => {
+ const { queryByTestId } = render(
+ <UnsavedChangesModal
+ showModal={false}
+ onHide={() => {}}
+ handleSave={() => {}}
+ onConfirmNavigation={() => {}}
+ />,
+ );
+
+ expect(queryByTestId('unsaved-changes-modal')).not.toBeInTheDocument();
+});
+
+test('should render the UnsavedChangesModal component if showModal is true',
async () => {
+ const { findByTestId } = render(
+ <UnsavedChangesModal
+ showModal
+ onHide={() => {}}
+ handleSave={() => {}}
+ onConfirmNavigation={() => {}}
+ />,
+ );
+
+ expect(await findByTestId('unsaved-changes-modal')).toBeInTheDocument();
Review Comment:
Can we rely on roles whenever possible?
https://github.com/apache/superset/wiki/Testing-Guidelines-and-Best-Practices#keep-accessibility-in-mind-when-writing-your-tests
##########
superset-frontend/src/components/UnsavedChangesModal/index.tsx:
##########
@@ -0,0 +1,134 @@
+/**
+ * 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.
+ */
+import Modal from 'src/components/Modal';
+import Button from 'src/components/Button';
+import { t, styled, css } from '@superset-ui/core';
+import type { FC, ReactElement } from 'react';
+import { Icons } from 'src/components/Icons';
+
+const StyledModalTitle = styled.h1`
+ ${({ theme }) => css`
Review Comment:
We are trying to avoid as much custom styles as possible and go vanilla Ant
Design as we anticipate introducing the theming functionalities. Would it look
bad if we reduced custom styles to the minimum, especially avoiding
customizations of colors and fonts?
##########
superset-frontend/src/dashboard/components/Header/index.jsx:
##########
@@ -823,6 +835,15 @@ const Header = () => {
}
`}
/>
+
+ <UnsavedChangesModal
+ title="Save changes to your dashboard?"
Review Comment:
We might need to localize these with the `t` function here. If this happens
inside the component, it's still best for localization to be at the usage level.
--
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]
