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.

Sign up for GitHub

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

Status, Status Link Controls update #1580

Closed
wants to merge 1 commit into from
Closed

Status, Status Link Controls update #1580

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
54 changes: 37 additions & 17 deletions frontend/taipy-gui/src/components/Taipy/Status.spec.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -12,38 +12,58 @@
*/

import React from "react";
import {render} from "@testing-library/react";
import { render } from "@testing-library/react";
import "@testing-library/jest-dom";
import userEvent from "@testing-library/user-event";
import { getStatusIcon } from "./Status";

import { PlusOneOutlined } from "@mui/icons-material";
import Status, { StatusType } from "./Status";

import Status, { StatusType } from './Status';

const status: StatusType = {status: "status", message: "message"};
const status: StatusType = { status: "status", message: "message" };

describe("Status Component", () => {
it("renders", async () => {
const {getByText} = render(<Status value={status} />);
const { getByText } = render(<Status value={status} />);
const elt = getByText("message");
const av = getByText("S");
expect(elt.tagName).toBe("SPAN");
expect(av.tagName).toBe("DIV");
})
});
it("uses the class", async () => {
const {getByText} = render(<Status value={status} className="taipy-status" />);
const { getByText } = render(<Status value={status} className="taipy-status" />);
const elt = getByText("message");
expect(elt.parentElement).toHaveClass("taipy-status");
})
});
it("can be closed", async () => {
const myClose = jest.fn();
const {getByTestId} = render(<Status value={status} onClose={myClose} />);
const { getByTestId } = render(<Status value={status} onClose={myClose} />);
const elt = getByTestId("CancelIcon");
await userEvent.click(elt);
expect(myClose).toHaveBeenCalled();
})
it("displays the icon", async () => {
const {getByTestId} = render(<Status value={status} icon={<PlusOneOutlined/>} onClose={jest.fn()} />);
getByTestId("PlusOneOutlinedIcon");
})
});
});

describe("StatusAvatar Icon Rendering", () => {
it("renders the correct icon for 'S' status", () => {
const { getByTestId } = render(getStatusIcon("S"));
getByTestId("CheckCircleIcon");
});

it("renders the correct icon for 'E' status", () => {
const { getByTestId } = render(getStatusIcon("E"));
getByTestId("ErrorIcon");
});

it("renders the correct icon for 'W' status", () => {
const { getByTestId } = render(getStatusIcon("W"));
getByTestId("WarningIcon");
});

it("renders the correct icon for 'I' status", () => {
const { getByTestId } = render(getStatusIcon("I"));
getByTestId("InfoIcon");
});

it("renders the default emoji for unknown status", () => {
const { getByText } = render(getStatusIcon("unknown"));
getByText("❓");
});
});
21 changes: 20 additions & 1 deletion frontend/taipy-gui/src/components/Taipy/Status.tsx
View file
Open in desktop
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should we allow the choice between letter and icon, for example add a property with_icon boolean True by default.
What do you think @FlorianJacta @FabienLelaquais ?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I guess, we should

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

please advise on the next steps

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What kind of advice are you looking for?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

  • add a property to taipy\gui_renderers\factory.py L 494 : ("with_icon", PropertyType.boolean, False),
  • add the corresponding property to frontend\taipy-gui\src\components\Taipy\StatusList.tsx L 88: withIcon?: boolean;
  • declare a default value L92: withIcon = false
  • pass the value to the Status Component L 170 and L 161 : withIcon={withIcon}
  • in frontend\taipy-gui\src\components\Taipy\Status.tsx declare and use the withIcon
  • add tests to StatusList and Status

Copy link
Member

@FabienLelaquais FabienLelaquais Aug 10, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@pravintargaryen @FredLL-Avaiga @FlorianJacta
Before a proposal for implementation is done, I would like to have some requirements.
I really don't like the with_icon proposal that addresses only the issue (which is good) but makes little sense for future improvements.

This 'all or nothing' approach sounds really bad to me. We should let users decide what to show for what status priority.
Maybe some application has a special need where a new status priority would be useful as well (say the application enters a disastrous mode, where users would want to create a status line like status = ("disaster", "Out of memory.").
Each status priority could have an associated icon, accessed in a dictionary for example.

So the default would be a dict:

    icons = { 
        "S": <success-icon>,
        "E": <error-icon>,
        "W":  <warning-icon>,
        "I":  <info-icon>
     }

and the "question mark" fallback, we would let users have the possibility of customizing the status priority representation by overloading existing ones or adding their own.

My first proposal would then be a new property called icons, holding something that would be a dict that merges with the predefined one.

  • Each key would be a status priority.
    When a new status record arrives, we search for a corresponding key: one that, case-insensitively, would best match the recode status priority (today, the rule is 'starts with', which is fine — we can think of something better like 'most matching starting characters').
  • Each value would be an Icon object, that we already have all over the place in Taipy.

The result is:

  • The status control represents status priorities with a relevant icon by default
  • If a user prefers the text version, then the property must be set to a dict that overloads all priority ids with None
  • If an application requires a new status priority, it can be decorated using the property.

I'm still not clear on the 'icon selection' rule - but it can be addressed by really listing use cases.

My final point are:

  • do not rush implementing features that are unclear or limiting.
  • why invent a new type of Icon in Taipy? Or can Icon rely on MaterialUI icons with some sorcellery?

Comments are welcome.
Code, not just yet.

Original file line number Diff line number Diff line change
Expand Up @@ -14,6 +14,10 @@
import React, { MouseEvent, ReactNode, useMemo } from "react";
import Chip from "@mui/material/Chip";
import Avatar from "@mui/material/Avatar";
import CheckCircleIcon from "@mui/icons-material/CheckCircle";
import ErrorIcon from "@mui/icons-material/Error";
import WarningIcon from "@mui/icons-material/Warning";
import InfoIcon from "@mui/icons-material/Info";

import { getInitials } from "../../utils";
import { TaipyBaseProps } from "./utils";
Expand All @@ -30,6 +34,21 @@ interface StatusProps extends TaipyBaseProps {
icon?: ReactNode;
}

export const getStatusIcon = (status: string) => {
switch (status) {
case "S":
return <CheckCircleIcon data-testid="CheckCircleIcon" />;
case "E":
return <ErrorIcon data-testid="ErrorIcon" />;
case "W":
return <WarningIcon data-testid="WarningIcon" />;
case "I":
return <InfoIcon data-testid="InfoIcon" />;
default:
return "❓";
}
};

const status2Color = (status: string): "error" | "info" | "success" | "warning" => {
status = (status || "").toLowerCase();
status = status.length == 0 ? " " : status.charAt(0);
Expand All @@ -54,7 +73,7 @@ const Status = (props: StatusProps) => {
const chipProps = useMemo(() => {
const cp: Record<string, unknown> = {};
cp.color = status2Color(value.status);
cp.avatar = <Avatar sx={{ bgcolor: `${cp.color}.main` }}>{getInitials(value.status)}</Avatar>;
cp.avatar = <Avatar sx={{ bgcolor: `${cp.color}.main` }}>{getStatusIcon(getInitials(value.status))}</Avatar>;
if (props.onClose) {
cp.onDelete = props.onClose;
}
Expand Down
41 changes: 19 additions & 22 deletions frontend/taipy-gui/src/components/Taipy/StatusList.spec.tsx
View file
Open in desktop
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No need to commit a file if it is only reformated

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

please advise on the next steps

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

you'll have to modify it to test the new property

Original file line number Diff line number Diff line change
Expand Up @@ -12,60 +12,57 @@
*/

import React from "react";
import {render} from "@testing-library/react";
import { render } from "@testing-library/react";
import "@testing-library/jest-dom";
import userEvent from "@testing-library/user-event";

import { StatusType } from "./Status";
import StatusList from './StatusList';
import StatusList from "./StatusList";

const statuses = [
{status: "info", message: "info"},
{ status: "info", message: "info" },
["error", "error"],
{status: "warning", message: "warning"},
{status: "success", message: "success"},
{ status: "warning", message: "warning" },
{ status: "success", message: "success" },
] as Array<[string, string] | StatusType>;

describe("StatusList Component", () => {
it("renders", async () => {
const {getByText} = render(<StatusList value={statuses} />);
const { getByText } = render(<StatusList value={statuses} />);
const elt = getByText("4 statuses");
const av = getByText("E");
expect(elt.tagName).toBe("SPAN");
expect(av.tagName).toBe("DIV");
})
});
it("uses the class", async () => {
const {getByText} = render(<StatusList value={statuses} className="taipy-status" />);
const { getByText } = render(<StatusList value={statuses} className="taipy-status" />);
const elt = getByText("4 statuses");
expect(elt.parentElement).toHaveClass("taipy-status");
})
});
it("can be opened when more than 1 status", async () => {
const {getByTestId} = render(<StatusList value={statuses} />);
const { getByTestId } = render(<StatusList value={statuses} />);
const elt = getByTestId("ArrowDownwardIcon");
})
});
it("cannot be opened when 1 status", async () => {
const {queryAllByRole} = render(<StatusList value={statuses[0]} />);
const { queryAllByRole } = render(<StatusList value={statuses[0]} />);
expect(queryAllByRole("button")).toHaveLength(0);
})
});
it("displays a default status", async () => {
const {getByText} = render(<StatusList value={[]} />);
const { getByText } = render(<StatusList value={[]} />);
getByText("No Status");
getByText("I");
})
});
it("opens on click", async () => {
const {getByTestId, getByText} = render(<StatusList value={statuses} />);
const { getByTestId, getByText } = render(<StatusList value={statuses} />);
const elt = getByTestId("ArrowDownwardIcon");
await userEvent.click(elt);
const selt = getByText("info");
expect(selt.parentElement?.parentElement?.childElementCount).toBe(4);
})
});
it("hide closed statuses", async () => {
const {getByTestId, queryAllByTestId} = render(<StatusList value={statuses} />);
const { getByTestId, queryAllByTestId } = render(<StatusList value={statuses} />);
const elt = getByTestId("ArrowDownwardIcon");
await userEvent.click(elt);
const icons = queryAllByTestId("CancelIcon");
expect(icons).toHaveLength(4);
await userEvent.click(icons[0]);
expect(queryAllByTestId("CancelIcon")).toHaveLength(3);
})
});
});
Loading