Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat(explore): more toast feedback on user actions in Explore #18108

Merged
merged 7 commits into from
Jan 24, 2022
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
Expand Up @@ -48,6 +48,7 @@ type ExploreActionButtonsProps = {
queriesResponse: {};
slice: { slice_name: string };
addDangerToast: Function;
addSuccessToast: Function;
};

const VIZ_TYPES_PIVOTABLE = ['pivot_table', 'pivot_table_v2'];
Expand Down Expand Up @@ -98,6 +99,7 @@ const ExploreActionButtons = (props: ExploreActionButtonsProps) => {
latestQueryFormData,
slice,
addDangerToast,
addSuccessToast,
} = props;

const copyTooltipText = t('Copy chart URL to clipboard');
Expand All @@ -111,8 +113,10 @@ const ExploreActionButtons = (props: ExploreActionButtonsProps) => {
const shortUrl = await getShortUrl();
await copyTextToClipboard(shortUrl);
setCopyTooltip(t('Copied to clipboard!'));
addSuccessToast(t('Copied to clipboard!'));
} catch (error) {
setCopyTooltip(t('Sorry, your browser does not support copying.'));
addDangerToast(t('Sorry, your browser does not support copying.'));
}
};

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -22,7 +22,7 @@ import { Slice } from 'src/types/Chart';
import { render, screen, waitFor } from 'spec/helpers/testing-library';
import fetchMock from 'fetch-mock';
import userEvent from '@testing-library/user-event';
import PropertiesModal from '.';
import PropertiesModal, { PropertiesModalProps } from '.';

const createProps = () => ({
slice: {
Expand Down Expand Up @@ -68,6 +68,7 @@ const createProps = () => ({
show: true,
onHide: jest.fn(),
onSave: jest.fn(),
addSuccessToast: jest.fn(),
});

fetchMock.get('glob:*/api/v1/chart/318', {
Expand Down Expand Up @@ -160,10 +161,13 @@ afterAll(() => {
fetchMock.resetBehavior();
});

const renderModal = (props: PropertiesModalProps) =>
render(<PropertiesModal {...props} />, { useRedux: true });

test('Should render null when show:false', async () => {
const props = createProps();
props.show = false;
render(<PropertiesModal {...props} />);
renderModal(props);

await waitFor(() => {
expect(
Expand All @@ -174,7 +178,7 @@ test('Should render null when show:false', async () => {

test('Should render when show:true', async () => {
const props = createProps();
render(<PropertiesModal {...props} />);
renderModal(props);

await waitFor(() => {
expect(
Expand All @@ -185,7 +189,7 @@ test('Should render when show:true', async () => {

test('Should have modal header', async () => {
const props = createProps();
render(<PropertiesModal {...props} />);
renderModal(props);

await waitFor(() => {
expect(screen.getByText('Edit Chart Properties')).toBeVisible();
Expand All @@ -196,7 +200,7 @@ test('Should have modal header', async () => {

test('"Close" button should call "onHide"', async () => {
const props = createProps();
render(<PropertiesModal {...props} />);
renderModal(props);

await waitFor(() => {
expect(props.onHide).toBeCalledTimes(0);
Expand All @@ -212,7 +216,7 @@ test('"Close" button should call "onHide"', async () => {

test('Should render all elements inside modal', async () => {
const props = createProps();
render(<PropertiesModal {...props} />);
renderModal(props);
await waitFor(() => {
expect(screen.getAllByRole('textbox')).toHaveLength(5);
expect(screen.getByRole('combobox')).toBeInTheDocument();
Expand Down Expand Up @@ -240,7 +244,7 @@ test('Should render all elements inside modal', async () => {

test('Should have modal footer', async () => {
const props = createProps();
render(<PropertiesModal {...props} />);
renderModal(props);

await waitFor(() => {
expect(screen.getByText('Cancel')).toBeVisible();
Expand All @@ -254,7 +258,7 @@ test('Should have modal footer', async () => {

test('"Cancel" button should call "onHide"', async () => {
const props = createProps();
render(<PropertiesModal {...props} />);
renderModal(props);

await waitFor(() => {
expect(props.onHide).toBeCalledTimes(0);
Expand All @@ -270,7 +274,7 @@ test('"Cancel" button should call "onHide"', async () => {

test('"Save" button should call only "onSave"', async () => {
const props = createProps();
render(<PropertiesModal {...props} />);
renderModal(props);
await waitFor(() => {
expect(props.onSave).toBeCalledTimes(0);
expect(props.onHide).toBeCalledTimes(0);
Expand All @@ -294,7 +298,7 @@ test('Empty "Certified by" should clear "Certification details"', async () => {
certified_by: '',
},
};
render(<PropertiesModal {...noCertifiedByProps} />);
renderModal(noCertifiedByProps);

expect(
screen.getByRole('textbox', { name: 'Certification details' }),
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -26,14 +26,16 @@ import rison from 'rison';
import { t, SupersetClient, styled } from '@superset-ui/core';
import Chart, { Slice } from 'src/types/Chart';
import { getClientErrorObject } from 'src/utils/getClientErrorObject';
import withToasts from 'src/components/MessageToasts/withToasts';

type PropertiesModalProps = {
export type PropertiesModalProps = {
slice: Slice;
show: boolean;
onHide: () => void;
onSave: (chart: Chart) => void;
permissionsError?: string;
existingOwners?: SelectValue;
addSuccessToast: (msg: string) => void;
};

const FormItem = Form.Item;
Expand All @@ -46,11 +48,12 @@ const StyledHelpBlock = styled.span`
margin-bottom: 0;
`;

export default function PropertiesModal({
function PropertiesModal({
slice,
onHide,
onSave,
show,
addSuccessToast,
}: PropertiesModalProps) {
const [submitting, setSubmitting] = useState(false);
const [form] = Form.useForm();
Expand Down Expand Up @@ -157,6 +160,7 @@ export default function PropertiesModal({
id: slice.slice_id,
};
onSave(updatedChart);
addSuccessToast(t('Chart properties updated'));
onHide();
} catch (res) {
const clientError = await getClientErrorObject(res);
Expand Down Expand Up @@ -308,3 +312,5 @@ export default function PropertiesModal({
</Modal>
);
}

export default withToasts(PropertiesModal);
8 changes: 4 additions & 4 deletions superset/views/core.py
Original file line number Diff line number Diff line change
Expand Up @@ -975,11 +975,11 @@ def save_or_overwrite_slice(
if action == "saveas" and slice_add_perm:
ChartDAO.save(slc)
msg = _("Chart [{}] has been saved").format(slc.slice_name)
flash(msg, "info")
flash(msg, "success")
elif action == "overwrite" and slice_overwrite_perm:
ChartDAO.overwrite(slc)
msg = _("Chart [{}] has been overwritten").format(slc.slice_name)
flash(msg, "info")
flash(msg, "success")

# Adding slice to a dashboard if requested
dash: Optional[Dashboard] = None
Expand Down Expand Up @@ -1008,7 +1008,7 @@ def save_or_overwrite_slice(
_("Chart [{}] was added to dashboard [{}]").format(
slc.slice_name, dash.dashboard_title
),
"info",
"success",
)
elif new_dashboard_name:
# Creating and adding to a new dashboard
Expand All @@ -1030,7 +1030,7 @@ def save_or_overwrite_slice(
_(
"Dashboard [{}] just got created and chart [{}] was added " "to it"
).format(dash.dashboard_title, slc.slice_name),
"info",
"success",
)

if dash and slc not in dash.slices:
Expand Down