Skip to content
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
@@ -0,0 +1,26 @@
import React, { useCallback } from "react";
import { MenuItem } from "@appsmith/ads";
import { createMessage, DOCUMENTATION } from "ee/constants/messages";
import { DocsLink, openDoc } from "constants/DocumentationLinks";
import { usePluginActionContext } from "../../../PluginActionContext";

export const DocsMenuItem = () => {
const { plugin } = usePluginActionContext();
const onDocsClick = useCallback(() => {
openDoc(DocsLink.QUERY, plugin.documentationLink, plugin.name);
}, [plugin]);

if (!plugin.documentationLink) {
return null;
}

return (
<MenuItem
className="t--datasource-documentation-link"
onSelect={onDocsClick}
startIcon="book-line"
>
{createMessage(DOCUMENTATION)}
</MenuItem>
);
};
2 changes: 2 additions & 0 deletions app/client/src/PluginActionEditor/index.ts
Original file line number Diff line number Diff line change
Expand Up @@ -13,3 +13,5 @@ export type {
export { default as PluginActionNameEditor } from "./components/PluginActionNameEditor";

export type { PluginActionEditorState } from "./store/pluginEditorReducer";

export { DocsMenuItem } from "./components/PluginActionToolbar/components/DocsMenuItem";
Original file line number Diff line number Diff line change
@@ -1,9 +1,9 @@
import React from "react";
import { PluginActionToolbar } from "PluginActionEditor";
import AppPluginActionMenu from "./PluginActionMoreActions";
import { ToolbarMenu } from "./ToolbarMenu";

const AppPluginActionToolbar = () => {
return <PluginActionToolbar menuContent={<AppPluginActionMenu />} />;
return <PluginActionToolbar menuContent={<ToolbarMenu />} />;
};

export default AppPluginActionToolbar;

This file was deleted.

Original file line number Diff line number Diff line change
@@ -0,0 +1,45 @@
import { useDispatch, useSelector } from "react-redux";
import { getPageList } from "ee/selectors/entitiesSelector";
import { usePluginActionContext } from "PluginActionEditor";
import React, { useCallback } from "react";
import { copyActionRequest } from "actions/pluginActionActions";
import { MenuSub, MenuSubContent, MenuSubTrigger } from "@appsmith/ads";
import { CONTEXT_COPY, createMessage } from "ee/constants/messages";
import { PageMenuItem } from "./PageMenuItem";

export const Copy = () => {
const menuPages = useSelector(getPageList);
const { action } = usePluginActionContext();
const dispatch = useDispatch();

const copyActionToPage = useCallback(
(pageId: string) =>
dispatch(
copyActionRequest({
id: action.id,
destinationPageId: pageId,
name: action.name,
}),
),
[action.id, action.name, dispatch],
);

return (
<MenuSub>
<MenuSubTrigger startIcon="duplicate">
{createMessage(CONTEXT_COPY)}
</MenuSubTrigger>
<MenuSubContent>
{menuPages.map((page) => {
return (
<PageMenuItem
key={page.basePageId}
onSelect={copyActionToPage}
page={page}
/>
);
})}
</MenuSubContent>
</MenuSub>
);
};
Original file line number Diff line number Diff line change
@@ -0,0 +1,35 @@
import { useHandleDeleteClick } from "PluginActionEditor/hooks";
import React, { useCallback, useState } from "react";
import {
CONFIRM_CONTEXT_DELETE,
CONTEXT_DELETE,
createMessage,
} from "ee/constants/messages";
import { MenuItem } from "@appsmith/ads";

export const Delete = () => {
const { handleDeleteClick } = useHandleDeleteClick();
const [confirmDelete, setConfirmDelete] = useState(false);

const handleSelect = useCallback(
(e?: Event) => {
e?.preventDefault();
confirmDelete ? handleDeleteClick({}) : setConfirmDelete(true);
},
[confirmDelete, handleDeleteClick],
);

const menuLabel = confirmDelete
? createMessage(CONFIRM_CONTEXT_DELETE)
: createMessage(CONTEXT_DELETE);

return (
<MenuItem
className="t--apiFormDeleteBtn error-menuitem"
onSelect={handleSelect}
startIcon="trash"
>
{menuLabel}
</MenuItem>
);
};
Comment on lines +22 to +35
Copy link
Contributor

Choose a reason for hiding this comment

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

🛠️ Refactor suggestion

Your rendering code is looking sharp, but let's add a finishing touch!

The use of createMessage for labels and the dynamic rendering based on confirmDelete state show great attention to detail. However, let's enhance the accessibility of our component:

Add an aria-label to the MenuItem to improve screen reader support. Here's how you can do it:

<MenuItem
  className="t--apiFormDeleteBtn error-menuitem"
  onSelect={handleSelect}
  startIcon="trash"
  aria-label={confirmDelete ? "Confirm delete action" : "Delete action"}
>
  {menuLabel}
</MenuItem>

This small addition will make your component more inclusive and user-friendly. Keep striving for excellence!

Original file line number Diff line number Diff line change
@@ -0,0 +1,61 @@
import { useDispatch, useSelector } from "react-redux";
import { usePluginActionContext } from "PluginActionEditor";
import { getCurrentPageId } from "selectors/editorSelectors";
import { getPageList } from "ee/selectors/entitiesSelector";
import React, { useCallback, useMemo } from "react";
import { moveActionRequest } from "actions/pluginActionActions";
import {
MenuItem,
MenuSub,
MenuSubContent,
MenuSubTrigger,
} from "@appsmith/ads";
import { CONTEXT_MOVE, createMessage } from "ee/constants/messages";
import { PageMenuItem } from "./PageMenuItem";

export const Move = () => {
const dispatch = useDispatch();
const { action } = usePluginActionContext();

const currentPageId = useSelector(getCurrentPageId);
const allPages = useSelector(getPageList);
const menuPages = useMemo(() => {
return allPages.filter((page) => page.pageId !== currentPageId);
}, [allPages, currentPageId]);

const moveActionToPage = useCallback(
(destinationPageId: string) =>
dispatch(
moveActionRequest({
id: action.id,
destinationPageId,
originalPageId: currentPageId,
name: action.name,
}),
),
[dispatch, action.id, action.name, currentPageId],
);

return (
<MenuSub>
<MenuSubTrigger startIcon="swap-horizontal">
{createMessage(CONTEXT_MOVE)}
</MenuSubTrigger>
<MenuSubContent>
{menuPages.length > 1 ? (
menuPages.map((page) => {
return (
<PageMenuItem
key={page.basePageId}
onSelect={moveActionToPage}
page={page}
/>
);
})
) : (
<MenuItem key="no-pages">No pages</MenuItem>
)}
</MenuSubContent>
</MenuSub>
);
};
Original file line number Diff line number Diff line change
@@ -0,0 +1,14 @@
import type { Page } from "entities/Page";
import React, { useCallback } from "react";
import { MenuItem } from "@appsmith/ads";

export const PageMenuItem = (props: {
page: Page;
onSelect: (id: string) => void;
}) => {
Comment on lines +5 to +8
Copy link
Contributor

Choose a reason for hiding this comment

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

🛠️ Refactor suggestion

Let's enhance our type definitions, shall we?

While your current approach is correct, I'd like to see you take it a step further. Consider defining an interface for your props. This will make your code more readable and reusable. Here's how you might do it:

interface PageMenuItemProps {
  page: Page;
  onSelect: (id: string) => void;
}

export const PageMenuItem: React.FC<PageMenuItemProps> = (props) => {
  // ... rest of the component
}

This change will make your code more organized and easier to maintain as your component grows. What do you think about this suggestion?

const handleOnSelect = useCallback(() => {
props.onSelect(props.page.pageId);
}, [props]);
Comment on lines +9 to +11
Copy link
Contributor

Choose a reason for hiding this comment

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

🛠️ Refactor suggestion

Good use of useCallback, but let's optimize it further!

I'm pleased to see you're thinking about performance with useCallback. However, we can make it even better. Instead of depending on the entire props object, let's be more specific:

const handleOnSelect = useCallback(() => {
  onSelect(page.pageId);
}, [onSelect, page.pageId]);

This way, the function will only be recreated if onSelect or page.pageId changes, not for any change in the props object. It's like sharpening your pencil to write more precisely!


return <MenuItem onSelect={handleOnSelect}>{props.page.pageName}</MenuItem>;
Copy link
Contributor

Choose a reason for hiding this comment

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

🛠️ Refactor suggestion

Let's make our menu item more accessible!

Your render method is neat and tidy, which is great! But remember, we want to make sure all our students can use our application. Let's add an aria-label to our MenuItem to improve accessibility:

return (
  <MenuItem 
    onSelect={handleOnSelect} 
    aria-label={`Go to page ${props.page.pageName}`}
  >
    {props.page.pageName}
  </MenuItem>
);

This small addition will help screen reader users understand the purpose of this menu item better. It's like adding a helpful sign to guide everyone to the right page!

};
Loading