chore: Update JS Editor Run designs #36998
Conversation
WalkthroughThe changes in this pull request involve multiple modifications across various components related to the JavaScript editor. Key updates include the introduction of a new Changes
Assessment against linked issues
Possibly related PRs
Suggested labels
Suggested reviewers
📜 Recent review detailsConfiguration used: .coderabbit.yaml 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
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
the
skip-changelog
app/client/src/pages/Editor/JSEditor/JSEditorToolbar/components/MenuTitle.tsx
Outdated
Show resolved
Hide resolved
app/client/src/pages/Editor/JSEditor/JSEditorToolbar/components/JSFunctionRun.tsx
Outdated
Show resolved
Hide resolved
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (1)
app/client/src/pages/Editor/JSEditor/JSEditorToolbar/JSEditorToolbar.tsx (1)
77-88: LGTM: Settings rendering logic improved with popover.The use of
ToolbarSettingsPopoverenhances the UI and user experience. The state management and localization are implemented correctly.Consider extracting the
createMessage(JS_EDITOR_SETTINGS.TITLE)call to a constant at the top of the component for better readability and potential reuse.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (2)
- app/client/src/pages/Editor/JSEditor/JSEditorToolbar/JSEditorToolbar.tsx (4 hunks)
- app/client/src/pages/Editor/JSEditor/JSEditorToolbar/components/JSFunctionSettings.tsx (1 hunks)
🧰 Additional context used
🔇 Additional comments (3)
app/client/src/pages/Editor/JSEditor/JSEditorToolbar/JSEditorToolbar.tsx (3)
1-1: LGTM: Import statements updated appropriately.The new imports align with the component's enhanced functionality.
Also applies to: 2-2, 11-11
52-53: LGTM: State variable for popover visibility added.The
isOpenstate is appropriately initialized for managing the settings popover visibility.
Line range hint
1-95: LGTM: Component structure maintained with focused improvements.The changes enhance the settings UI while maintaining the overall component structure and feature flag logic. This approach allows for a smooth transition to the new design.
app/client/src/pages/Editor/JSEditor/JSEditorToolbar/components/JSFunctionSettings.tsx
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (1)
app/client/src/pages/Editor/JSEditor/JSEditorToolbar/components/JSFunctionSettings.test.tsx (1)
3-3: LGTM. Consider adding a test for feature flag disabled state.The removal of button click events aligns with the component's new behavior. Good job on maintaining test coverage for the existing scenarios.
Consider adding a test case to verify the component's behavior when the feature flag is disabled.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (4)
- app/client/src/pages/Editor/JSEditor/JSEditorToolbar/components/JSFunctionItem.tsx (1 hunks)
- app/client/src/pages/Editor/JSEditor/JSEditorToolbar/components/JSFunctionRun.tsx (3 hunks)
- app/client/src/pages/Editor/JSEditor/JSEditorToolbar/components/JSFunctionSettings.test.tsx (1 hunks)
- app/client/src/pages/Editor/JSEditor/JSEditorToolbar/components/styled.ts (1 hunks)
✅ Files skipped from review due to trivial changes (1)
- app/client/src/pages/Editor/JSEditor/JSEditorToolbar/components/styled.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- app/client/src/pages/Editor/JSEditor/JSEditorToolbar/components/JSFunctionRun.tsx
🧰 Additional context used
🔇 Additional comments (3)
app/client/src/pages/Editor/JSEditor/JSEditorToolbar/components/JSFunctionItem.tsx (3)
1-12: LGTM: Imports and component structure.The imports are appropriate, and the component structure follows React best practices with proper TypeScript typing.
13-22: LGTM: Component logic and rendering.The use of
useCallbackfor memoization is a good practice. The rendering logic is clean and correctly implements theMenuItemwith the provided option.
1-22: Overall assessment: Well-implemented component.The
JSFunctionItemcomponent is concise, follows React best practices, and effectively encapsulates the functionality of rendering a menu item for a JS function. Good job on this implementation.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (1)
app/client/packages/design-system/ads/src/Icon/Icon.provider.tsx (1)
1067-1070: LGTM. Consider grouping related icons.The addition of
ExternalLinkIconlooks good. For better organization, consider grouping related icons together in theICON_LOOKUPobject.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
⛔ Files ignored due to path filters (1)
app/client/packages/design-system/ads/src/__assets__/icons/ads/external-link-line.svgis excluded by!**/*.svg
📒 Files selected for processing (1)
- app/client/packages/design-system/ads/src/Icon/Icon.provider.tsx (3 hunks)
🧰 Additional context used
🔇 Additional comments (1)
app/client/packages/design-system/ads/src/Icon/Icon.provider.tsx (1)
1219-1219: Maintain alphabetical order in ICON_LOOKUP.The new entry
"external-link-line": ExternalLinkIcon,is correctly placed in alphabetical order. Good job maintaining the existing structure.
app/client/src/pages/Editor/JSEditor/JSEditorToolbar/components/styled.ts
Outdated
Show resolved
Hide resolved
github-actions
bot
added
IDE Navigation
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (1)
app/client/src/pages/Editor/JSEditor/JSEditorToolbar/components/styles.ts (1)
4-9: Consider adding TypeScript types and improving CSS custom property naming.Two suggestions for improvement:
- The CSS custom property
--button-font-weightseems misnamed as this is a menu title component- Consider adding TypeScript type definitions for the styled component
-export const MenuTitle = styled(Text)` +export const MenuTitle = styled(Text)<{ maxWidth?: string }>` --menu-title-font-weight: bold; overflow: hidden; text-overflow: ellipsis; - max-width: 30ch; + max-width: ${({ maxWidth = "30ch" }) => maxWidth};
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (3)
- app/client/src/PluginActionEditor/components/PluginActionSettings/SettingsPopover.tsx (1 hunks)
- app/client/src/pages/Editor/JSEditor/JSEditorToolbar/components/JSFunctionItem.tsx (1 hunks)
- app/client/src/pages/Editor/JSEditor/JSEditorToolbar/components/styles.ts (1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- app/client/src/pages/Editor/JSEditor/JSEditorToolbar/components/JSFunctionItem.tsx
🧰 Additional context used
🔇 Additional comments (3)
app/client/src/pages/Editor/JSEditor/JSEditorToolbar/components/styles.ts (1)
6-8: LGTM: Proper text truncation implementation.The text truncation pattern using overflow and text-overflow properties is correctly implemented.
app/client/src/PluginActionEditor/components/PluginActionSettings/SettingsPopover.tsx (2)
Line range hint
1-105: Implementation follows best practices.The component demonstrates clean code organization, proper hook usage, and type safety.
91-91: Icon change aligns with design system update.The change from "share-box-line" to "external-link-line" is consistent with the PR's design system updates.
Let's verify the icon exists in the design system:
✅ Verification successful
Let me verify the icon implementation in the design system files to ensure it's properly defined.
The above script will help us see the actual implementation and confirm if the icon is properly defined in the design system. Let's examine the context around its usage to ensure it's a valid icon name.
Icon change is valid and properly defined in the design system
The "external-link-line" icon is correctly defined in both the new and old design system:
- Imported as SVG in
ads/src/Icon/Icon.provider.tsx- Mapped in the icon providers of both design systems
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify the external-link-line icon exists in the design system # Expected: Find references to the external-link-line icon definition rg -l "external-link-line"Length of output: 242
Script:
#!/bin/bash # Check the icon implementation in the design system files rg "external-link-line" app/client/packages/design-system/ -A 5 -B 5Length of output: 3308
app/client/src/pages/Editor/JSEditor/JSEditorToolbar/components/JSFunctionSettings.tsx
Show resolved
Hide resolved
## Description - Update the icons in ADS - Override the Menu Title text styles ## Automation /ok-to-test tags="@tag.JS" ### 🔍 Cypress test results <!-- This is an auto-generated comment: Cypress test results --> > [!TIP] > 🟢 🟢 🟢 All cypress tests have passed! 🎉 🎉 🎉 > Workflow run: <https://github.com/appsmithorg/appsmith/actions/runs/11440728547> > Commit: 1df24c6 > <a href="https://internal.appsmith.com/app/cypress-dashboard/rundetails-65890b3c81d7400d08fa9ee5?branch=master&workflowId=11440728547&attempt=1" target="_blank">Cypress dashboard</a>. > Tags: `@tag.JS` > Spec: > <hr>Mon, 21 Oct 2024 13:43:16 UTC <!-- end of auto-generated comment: Cypress test results --> ## Communication Should the DevRel and Marketing teams inform users about this change? - [ ] Yes - [ ] No <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit - **New Features** - Introduced a new styled component, `MenuTitle`, for improved dropdown menu item labeling. - Enhanced the `JSFunctionRun` component with a new button icon and improved function selection handling. - **Bug Fixes** - Updated the callback function to ensure it correctly responds to changes in the `onSelect` prop. <!-- end of auto-generated comment: release notes by coderabbit.ai -->
Description
Fixes #35528
Fixes #35509
Fixes #34323
amongst other things
Automation
/ok-to-test tags="@tag.JS"
🔍 Cypress test results
Tip
🟢 🟢 🟢 All cypress tests have passed! 🎉 🎉 🎉
Workflow run: https://github.com/appsmithorg/appsmith/actions/runs/11474060916
Commit: 41ed8e2
Cypress dashboard.
Tags:
@tag.JSSpec:
Wed, 23 Oct 2024 06:47:16 UTC
Communication
Should the DevRel and Marketing teams inform users about this change?
Summary by CodeRabbit
Release Notes
New Features
JSFunctionItemcomponent for improved dropdown functionality.ToolbarSettingsPopoverfor enhanced settings management in theJSEditorToolbar.ExternalLinkIconadded to the icon library for better visual representation.Improvements
JSFunctionSettingscomponent by removing the popover functionality, making settings always visible when enabled.endIconinPluginActionSettingsPopoverfor a more relevant icon display.disabledproperty to theToolbarSettingsPopoverfor better control over button interactivity.Style Changes
PluginActionFormfor improved layout.MenuTitlefor better text presentation in dropdown menus.Bug Fixes
JSFunctionSettingstests to enhance test accuracy.