chore: Docs in Plugin Action Toolbar#36881
Conversation
WalkthroughThis pull request introduces several new components and modifies existing ones within the Plugin Action Editor. A new Changes
Assessment against linked issues
Possibly related PRs
Suggested labels
Suggested reviewers
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
Documentation and Community
|
github-actions
bot
added
IDE Navigation
hetunandu
requested review from
AmanAgarwal041,
albinAppsmith and
ankitakinger
github-actions
bot
added
the
skip-changelog
|
This PR has increased the number of cyclic dependencies by 1, when compared with the release branch. Refer this document to identify the cyclic dependencies introduced by this PR. |
hetunandu
added
ok-to-test
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Outside diff range and nitpick comments (6)
app/client/src/PluginActionEditor/components/PluginActionToolbar/components/DocsMenuItem.tsx (2)
8-11: Excellent use of hooks, students! But let's add a small improvement.Your use of
usePluginActionContextanduseCallbackis commendable. However, let's make sure we're being thorough with our dependencies.Consider adding
plugin.documentationLinkandplugin.nameto the dependency array ofuseCallback:const onDocsClick = useCallback(() => { openDoc(DocsLink.QUERY, plugin.documentationLink, plugin.name); -}, [plugin]); +}, [plugin.documentationLink, plugin.name]);This ensures the callback is only recreated when these specific properties change, not the entire plugin object.
17-26: Wonderful job on rendering the MenuItem, students! Let's add a small enhancement.Your use of the MenuItem component is correct, and I'm pleased to see you've included a className for testing purposes. The localization implementation is also top-notch!
To make our component more accessible, let's add an
aria-labelto the MenuItem. This will help screen readers better understand the purpose of this menu item:<MenuItem className="t--datasource-documentation-link" onSelect={onDocsClick} startIcon="book-line" + aria-label={`Open ${plugin.name} documentation`} > {createMessage(DOCUMENTATION)} </MenuItem>This addition will earn you extra credit in web accessibility!
app/client/src/ce/pages/Editor/AppPluginActionEditor/components/ToolbarMenu/Delete.tsx (1)
14-20: Your handleSelect function is well-crafted, but there's room for improvement!The use of
useCallbackand the logic for handling both initial delete and confirmation are excellent. However, let's make it even better:Consider extracting the logic inside
handleSelectinto a separate function for improved readability. Here's an example:const performDeleteAction = useCallback(() => { confirmDelete ? handleDeleteClick({}) : setConfirmDelete(true); }, [confirmDelete, handleDeleteClick]); const handleSelect = useCallback( (e?: Event) => { e?.preventDefault(); performDeleteAction(); }, [performDeleteAction] );This separation of concerns will make your code even more clear and maintainable. Keep up the great work!
app/client/src/ce/pages/Editor/AppPluginActionEditor/components/ToolbarMenu/ToolbarMenu.tsx (2)
18-29: Let's add some helpful notes to our code, shall we?Class, your code is looking good, but remember: clear explanations help everyone understand better. Let's add some comments to explain what these permission calculations mean. It's like adding labels to our science experiments!
Here's a little homework for you:
export const ToolbarMenu = () => { const { action } = usePluginActionContext(); const isFeatureEnabled = useFeatureFlag(FEATURE_FLAG.license_gac_enabled); + // Check if the user has permission to manage actions const isChangePermitted = getHasManageActionPermission( isFeatureEnabled, action.userPermissions, ); + // Check if the user has permission to delete actions const isDeletePermitted = getHasDeleteActionPermission( isFeatureEnabled, action?.userPermissions, );
31-44: Excellent job on your component structure, students!Your render logic is as clear as a well-drawn diagram on our classroom board. I'm particularly impressed with how you've used conditional rendering to show different options based on user permissions. It's like choosing the right tools for each experiment!
To make it even clearer, let's add a small improvement:
return ( <> <ConvertToModuleCTA /> {isChangePermitted && ( <> <Copy /> <Move /> </> )} <Docs /> <MenuSeparator /> - {isDeletePermitted && <Delete />} + {isDeletePermitted && ( + <Delete /> + )} </> );This change makes the
Deletecomponent rendering consistent with the other conditional renders. Remember, consistency is key in both code and classroom rules!app/client/src/ce/pages/Editor/AppPluginActionEditor/components/ToolbarMenu/Move.tsx (1)
39-61: Your render function gets an A+!Class, gather around and observe this exemplary render function. Notice how it elegantly handles both cases: when we have pages to move to, and when we don't. The use of the
mapfunction to create our menu items is particularly praiseworthy. And don't forget the importance of thatkeyprop – it's crucial for React's reconciliation process!One small suggestion for extra credit: Consider adding a more descriptive message when there are no pages available. For example:
<MenuItem key="no-pages">No other pages available for move</MenuItem>This would provide clearer feedback to our users. What do you think?
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (10)
- app/client/src/PluginActionEditor/components/PluginActionToolbar/components/DocsMenuItem.tsx (1 hunks)
- app/client/src/PluginActionEditor/index.ts (1 hunks)
- app/client/src/ce/pages/Editor/AppPluginActionEditor/components/AppPluginActionToolbar.tsx (1 hunks)
- app/client/src/ce/pages/Editor/AppPluginActionEditor/components/PluginActionMoreActions.tsx (0 hunks)
- app/client/src/ce/pages/Editor/AppPluginActionEditor/components/ToolbarMenu/Copy.tsx (1 hunks)
- app/client/src/ce/pages/Editor/AppPluginActionEditor/components/ToolbarMenu/Delete.tsx (1 hunks)
- app/client/src/ce/pages/Editor/AppPluginActionEditor/components/ToolbarMenu/Move.tsx (1 hunks)
- app/client/src/ce/pages/Editor/AppPluginActionEditor/components/ToolbarMenu/PageMenuItem.tsx (1 hunks)
- app/client/src/ce/pages/Editor/AppPluginActionEditor/components/ToolbarMenu/ToolbarMenu.tsx (1 hunks)
- app/client/src/ce/pages/Editor/AppPluginActionEditor/components/ToolbarMenu/index.ts (1 hunks)
💤 Files with no reviewable changes (1)
- app/client/src/ce/pages/Editor/AppPluginActionEditor/components/PluginActionMoreActions.tsx
✅ Files skipped from review due to trivial changes (1)
- app/client/src/ce/pages/Editor/AppPluginActionEditor/components/ToolbarMenu/index.ts
🧰 Additional context used
🔇 Additional comments (18)
app/client/src/ce/pages/Editor/AppPluginActionEditor/components/AppPluginActionToolbar.tsx (3)
Line range hint
1-10: Class, let's summarize what we've learned today!Excellent work on this assignment! You've successfully updated the
AppPluginActionToolbarcomponent to use the newToolbarMenu. This change aligns perfectly with our lesson plan of moving documentation into the overflow menu.Here's a quick recap of what we've covered:
- We've updated our import statement to bring in the new
ToolbarMenu.- We've integrated this new menu into our
PluginActionToolbar.These changes should improve the organization of our toolbar and make it easier for users to access documentation. Well done!
Remember, class, always double-check your work. Make sure the new
ToolbarMenucomponent includes all the necessary documentation links, and that it's functioning as expected in the overflow menu.
3-3: Class, let's examine this import statement change.Good job updating the import statement! This change aligns well with our lesson plan of moving documentation into the overflow menu. However, let's make sure we've done our homework.
To ensure we've covered all our bases, let's take a peek at the
ToolbarMenucomponent:
6-6: Now, class, let's turn our attention to this component rendering.Excellent work updating the
menuContentprop! You've successfully integrated our newToolbarMenucomponent. This change is like solving a math problem correctly - it all adds up!To ensure we haven't missed any important details, let's check the
PluginActionToolbarcomponent's props:✅ Verification successful
Great job persisting through the challenges! Let's refine our search to ensure we locate every instance of the
PluginActionToolbarcomponent. Please run the following script to search within.tsxand.tsfiles:
Excellent work!
I've verified that the
PluginActionToolbarcomponent'smenuContentprop correctly accepts the new<ToolbarMenu />component. Everything looks good, and the change aligns with our component definitions.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify the props of the PluginActionToolbar component # Test: Search for the PluginActionToolbar component definition and its props ast-grep --lang typescript --pattern 'export const PluginActionToolbar: React.FC<{ $$$ }> = ($_) => { $$$ }'Length of output: 114
Script:
#!/bin/bash # Description: Verify all export styles for the PluginActionToolbar component # Test: Search for both named and default exports of PluginActionToolbar ast-grep --lang typescript --pattern 'export const PluginActionToolbar: React.FC<{ $$$ }> = ($_ ) => { $$$ }' ast-grep --lang typescript --pattern 'export default PluginActionToolbar'Length of output: 187
Script:
#!/bin/bash # Description: Search for all definitions and usages of PluginActionToolbar component in TypeScript files # Test: Use ripgrep to find all instances of PluginActionToolbar rg 'PluginActionToolbar' --type tsx,tsLength of output: 69
Script:
#!/bin/bash # Description: Search for all definitions and usages of PluginActionToolbar component in TypeScript files # Test: Use ripgrep to find all instances of PluginActionToolbar rg 'PluginActionToolbar' --type tsx --type tsLength of output: 73
Script:
#!/bin/bash # Description: Search for all definitions and usages of PluginActionToolbar component in TypeScript files # Test: Use ripgrep to find all instances of PluginActionToolbar in .tsx and .ts files rg 'PluginActionToolbar' -g '*.tsx' -g '*.ts'Length of output: 1481
app/client/src/ce/pages/Editor/AppPluginActionEditor/components/ToolbarMenu/PageMenuItem.tsx (1)
1-3: Well done on your imports, class!You've shown a good understanding of what's needed for this component. The imports are concise and relevant, demonstrating your ability to keep things tidy and efficient.
app/client/src/PluginActionEditor/index.ts (1)
16-17: Well done, class! This addition is spot on.I'm pleased to see you've added the export for
DocsMenuItem. This new component will certainly help our users access documentation more easily, just as we discussed in our lesson plan (PR objectives). Remember, children, organization is key in coding, and you've done a splendid job placing this new export at the end of the file. Keep up the good work!app/client/src/PluginActionEditor/components/PluginActionToolbar/components/DocsMenuItem.tsx (2)
1-7: Well done, class! The imports and component declaration look spot on.You've correctly imported the necessary modules and declared the component using modern React practices. Keep up the good work!
13-15: A gold star for your conditional rendering, class!Your use of a guard clause to return
nullwhenplugin.documentationLinkis not available is exemplary. This prevents unnecessary rendering and potential errors. It's like checking if you have all your school supplies before starting your homework!app/client/src/ce/pages/Editor/AppPluginActionEditor/components/ToolbarMenu/Delete.tsx (2)
1-10: Well done on your imports and component declaration, class!You've shown a good understanding of organizing your code. The imports are neatly arranged, and you've correctly used the named export for your Delete component. Keep up the good work!
11-12: Excellent use of hooks, students!Your application of the custom hook
useHandleDeleteClickand theuseStatehook for managing the confirmation state is commendable. This approach ensures a safe deletion process. Keep up this level of thoughtfulness in your code!app/client/src/ce/pages/Editor/AppPluginActionEditor/components/ToolbarMenu/ToolbarMenu.tsx (2)
1-16: Well done on organizing your imports, class!Your imports are neatly arranged and properly grouped. It's like seeing a well-organized bookshelf in our classroom library! Keep up the good work in maintaining clean and readable code.
1-45: A gold star for your excellent work, class!Your
ToolbarMenucomponent is a shining example of how to organize and structure React code. You've successfully added the documentation to the toolbar and implemented the logic for displaying different options based on user permissions. This is exactly what we were aiming for in our project objectives!Here's a quick recap of our lesson today:
- Your imports are well-organized.
- You've used hooks effectively to manage state and permissions.
- Your render logic is clear and follows best practices.
Remember the small improvements we discussed:
- Adding comments to explain permission calculations.
- Keeping the rendering of conditional components consistent.
Overall, this is fantastic work! You've addressed the task of moving documentation into the overflow menu beautifully. Keep up the great coding, and don't forget to apply these lessons in your future projects!
app/client/src/ce/pages/Editor/AppPluginActionEditor/components/ToolbarMenu/Copy.tsx (4)
1-8: Well done on your import statements, class!I'm pleased to see you've used named imports throughout. This is excellent practice! Named imports not only make your code more readable but also allow for better tree-shaking, which can lead to smaller bundle sizes. Keep up the good work!
10-13: Excellent use of functional components and hooks, students!Your
Copycomponent is a shining example of modern React practices. By using functional components and hooks likeuseSelectorandusePluginActionContext, you're embracing the latest and greatest in React development. This approach leads to more concise and easier-to-understand code. Keep up this fantastic work!
15-25: A gold star for yourcopyActionToPagefunction implementation!Your use of
useCallbackhere is exemplary. This hook helps optimize performance by memoizing the function, preventing unnecessary re-renders. It's especially useful in this context where the function is passed as a prop to child components.Remember, class,
useCallbackis most effective when used in conjunction withReact.memoor when passing callbacks to optimized child components that rely on reference equality to prevent unnecessary renders.Keep up this level of attention to performance optimization!
27-45: Excellent work on your render method, class!Your use of the
MenuSubstructure and mapping overmenuPagesto renderPageMenuItemcomponents is very well done. It's clear, concise, and follows React best practices.I'm particularly impressed by your attention to detail in providing a unique
keyprop (page.basePageId) when mapping over the pages. This is crucial for React's reconciliation process and helps maintain component state correctly when the list changes.As a reminder to the class: Always provide unique and stable keys when rendering lists in React. This helps React identify which items have changed, been added, or been removed, and plays a crucial role in maintaining component state and improving performance.
Keep up the excellent work!
app/client/src/ce/pages/Editor/AppPluginActionEditor/components/ToolbarMenu/Move.tsx (3)
1-14: Well done on your imports, class!I'm pleased to see you've organized your imports neatly. You've brought in all the necessary tools for our lesson on moving actions between pages. Keep up the good work!
16-24: Excellent use of hooks, students!Your
Movecomponent is a shining example of modern React practices. You've wisely useduseDispatchanduseSelectorto interact with our Redux store, anduseMemoto optimize our page filtering. It's like you've been paying attention in class!
26-37: A gold star for your callback function!Your
moveActionToPagefunction is a textbook example of how to useuseCallback. You've correctly included all the necessary dependencies in the array. The way you've constructed the payload for themoveActionRequestaction shows a deep understanding of our Redux architecture. Keep up this level of work, and you'll be teaching the class soon!
| export const PageMenuItem = (props: { | ||
| page: Page; | ||
| onSelect: (id: string) => void; | ||
| }) => { |
There was a problem hiding this comment.
🛠️ 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]); |
There was a problem hiding this comment.
🛠️ 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!
| props.onSelect(props.page.pageId); | ||
| }, [props]); | ||
|
|
||
| return <MenuItem onSelect={handleOnSelect}>{props.page.pageName}</MenuItem>; |
There was a problem hiding this comment.
🛠️ 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!
| const menuLabel = confirmDelete | ||
| ? createMessage(CONFIRM_CONTEXT_DELETE) | ||
| : createMessage(CONTEXT_DELETE); | ||
|
|
||
| return ( | ||
| <MenuItem | ||
| className="t--apiFormDeleteBtn error-menuitem" | ||
| onSelect={handleSelect} | ||
| startIcon="trash" | ||
| > | ||
| {menuLabel} | ||
| </MenuItem> | ||
| ); | ||
| }; |
There was a problem hiding this comment.
🛠️ 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!
|
/build-deploy-preview skip-test=true |
|
Deploying Your Preview: https://github.com/appsmithorg/appsmith/actions/runs/11346401310. |
|
@hetunandu Can you add EE Pr for this? |
|
Deploy-Preview-URL: https://ce-36881.dp.appsmith.com |
| {isChangePermitted && ( | ||
| <> | ||
| <Copy /> | ||
| <Move /> |
There was a problem hiding this comment.
is there a difference in the context menu for plugin action on CE and EE? Asking for understanding why this needs to be inside ce folder.
3 participants
Description
Adds docs inside the Plugin Action Toolbar
Fixes #35533
Automation
/ok-to-test tags="@tag.IDE"
🔍 Cypress test results
Tip
🟢 🟢 🟢 All cypress tests have passed! 🎉 🎉 🎉
Workflow run: https://github.com/appsmithorg/appsmith/actions/runs/11346174657
Commit: eca5320
Cypress dashboard.
Tags:
@tag.IDESpec:
Tue, 15 Oct 2024 13:29:36 UTC
Communication
Should the DevRel and Marketing teams inform users about this change?
Summary by CodeRabbit
New Features
DocsMenuItemfor easy access to plugin documentation.Copy,Delete,Move,PageMenuItem, andToolbarMenuto enhance user interaction with plugin actions.Bug Fixes
AppPluginActionMenuwithToolbarMenufor improved functionality.Chores
AppPluginActionMenucomponent to streamline the codebase.