chore: Move IDE Sidebar into ADS #38641
Conversation
WalkthroughThe pull request introduces modifications to the Changes
Possibly related PRs
Suggested labels
Suggested Reviewers
Poem
📜 Recent review detailsConfiguration used: .coderabbit.yaml 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
⏰ Context from checks skipped due to timeout of 90000ms (10)
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
github-actions
bot
added
the
skip-changelog
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (3)
app/client/packages/design-system/ads/src/Templates/Sidebar/SidebarButton/SidebarButton.test.tsx (1)
17-60: Add test coverage for tooltip and remaining conditions.While the current tests cover core functionality, consider adding tests for:
- Tooltip rendering and content
- Other conditions that will be added (mentioned in TODO)
- Title rendering
app/client/packages/design-system/ads/src/Templates/Sidebar/Sidebar.tsx (1)
7-15: Consider making the sidebar width configurable.The fixed width of 50px might not work well across different screen sizes and resolutions.
- width: 50px; + width: ${props => props.width || '50px'};app/client/packages/design-system/ads/src/Templates/Sidebar/SidebarButton/SidebarButton.tsx (1)
15-18: Create issue to track remaining condition implementations.The TODO comment indicates pending work for Error and Success conditions.
Would you like me to create a GitHub issue to track the implementation of the remaining conditions?
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (10)
app/client/packages/design-system/ads/src/Templates/Sidebar/Sidebar.tsx(1 hunks)app/client/packages/design-system/ads/src/Templates/Sidebar/SidebarButton/SidebarButton.test.tsx(1 hunks)app/client/packages/design-system/ads/src/Templates/Sidebar/SidebarButton/SidebarButton.tsx(1 hunks)app/client/packages/design-system/ads/src/Templates/Sidebar/SidebarButton/index.ts(1 hunks)app/client/packages/design-system/ads/src/Templates/Sidebar/enums.ts(1 hunks)app/client/packages/design-system/ads/src/Templates/Sidebar/index.tsx(1 hunks)app/client/packages/design-system/ads/src/Templates/index.ts(1 hunks)app/client/src/IDE/index.ts(0 hunks)app/client/src/ce/entities/IDE/constants.ts(1 hunks)app/client/src/pages/Editor/IDE/Sidebar.tsx(1 hunks)
💤 Files with no reviewable changes (1)
- app/client/src/IDE/index.ts
✅ Files skipped from review due to trivial changes (4)
- app/client/packages/design-system/ads/src/Templates/Sidebar/enums.ts
- app/client/packages/design-system/ads/src/Templates/Sidebar/SidebarButton/index.ts
- app/client/src/pages/Editor/IDE/Sidebar.tsx
- app/client/packages/design-system/ads/src/Templates/Sidebar/index.tsx
⏰ Context from checks skipped due to timeout of 90000ms (11)
- GitHub Check: perform-test / client-build / client-build
- GitHub Check: perform-test / server-build / server-unit-tests
- GitHub Check: perform-test / rts-build / build
- GitHub Check: client-unit-tests / client-unit-tests
- GitHub Check: client-lint / client-lint
- GitHub Check: client-check-cyclic-deps / check-cyclic-dependencies
- GitHub Check: client-build / client-build
- GitHub Check: client-prettier / prettier-check
- GitHub Check: chromatic-deployment
- GitHub Check: storybook-tests
- GitHub Check: chromatic-deployment
🔇 Additional comments (2)
app/client/packages/design-system/ads/src/Templates/index.ts (1)
3-3: LGTM! Clean export statement following the existing pattern.app/client/src/ce/entities/IDE/constants.ts (1)
24-24: LGTM! Import path update aligns with modularization.The updated import path reflects the move of IDE components to ADS, supporting better modularity.
Let's verify the new import path exists:
✅ Verification successful
Import path is valid and aligns with IDE component refactoring
The type
IDESidebarButtonexists in the design system package and the import path is correctly updated as part of moving IDE components to ADS.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Verify the new import path exists and exports IDESidebarButton ast-grep --pattern 'export type IDESidebarButton'Length of output: 130878
Script:
#!/bin/bash # Search for any mention of IDESidebarButton rg "IDESidebarButton" # Look for IDE sidebar related files fd -t f -e ts -e tsx "sidebar" app/client/src # Check if @appsmith/ads exists fd -t d "ads" app/client/srcLength of output: 2345
| return ( | ||
| <Container className="t--sidebar" id={props.id}> | ||
| <div> | ||
| {topButtons.map((button) => ( | ||
| <SidebarButton | ||
| condition={button.condition} | ||
| icon={button.icon} | ||
| key={button.state} | ||
| onClick={onClick} | ||
| selected={editorState === button.state} | ||
| testId={button.testId} | ||
| title={button.title} | ||
| tooltip={button.tooltip} | ||
| urlSuffix={button.urlSuffix} | ||
| /> | ||
| ))} | ||
| </div> | ||
| <div> | ||
| {bottomButtons.map((button) => ( | ||
| <SidebarButton | ||
| condition={button.condition} | ||
| icon={button.icon} | ||
| key={button.state} | ||
| onClick={onClick} | ||
| selected={editorState === button.state} | ||
| testId={button.testId} | ||
| title={button.title} | ||
| tooltip={button.tooltip} | ||
| urlSuffix={button.urlSuffix} | ||
| /> | ||
| ))} | ||
| </div> | ||
| </Container> | ||
| ); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add ARIA attributes and extract button rendering logic.
- Add navigation role for accessibility:
- <Container className="t--sidebar" id={props.id}>
+ <Container className="t--sidebar" id={props.id} role="navigation" aria-label="IDE Navigation">- Consider extracting the repeated button mapping logic into a reusable function to avoid duplication.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| return ( | |
| <Container className="t--sidebar" id={props.id}> | |
| <div> | |
| {topButtons.map((button) => ( | |
| <SidebarButton | |
| condition={button.condition} | |
| icon={button.icon} | |
| key={button.state} | |
| onClick={onClick} | |
| selected={editorState === button.state} | |
| testId={button.testId} | |
| title={button.title} | |
| tooltip={button.tooltip} | |
| urlSuffix={button.urlSuffix} | |
| /> | |
| ))} | |
| </div> | |
| <div> | |
| {bottomButtons.map((button) => ( | |
| <SidebarButton | |
| condition={button.condition} | |
| icon={button.icon} | |
| key={button.state} | |
| onClick={onClick} | |
| selected={editorState === button.state} | |
| testId={button.testId} | |
| title={button.title} | |
| tooltip={button.tooltip} | |
| urlSuffix={button.urlSuffix} | |
| /> | |
| ))} | |
| </div> | |
| </Container> | |
| ); | |
| return ( | |
| <Container className="t--sidebar" id={props.id} role="navigation" aria-label="IDE Navigation"> | |
| <div> | |
| {topButtons.map((button) => ( | |
| <SidebarButton | |
| condition={button.condition} | |
| icon={button.icon} | |
| key={button.state} | |
| onClick={onClick} | |
| selected={editorState === button.state} | |
| testId={button.testId} | |
| title={button.title} | |
| tooltip={button.tooltip} | |
| urlSuffix={button.urlSuffix} | |
| /> | |
| ))} | |
| </div> | |
| <div> | |
| {bottomButtons.map((button) => ( | |
| <SidebarButton | |
| condition={button.condition} | |
| icon={button.icon} | |
| key={button.state} | |
| onClick={onClick} | |
| selected={editorState === button.state} | |
| testId={button.testId} | |
| title={button.title} | |
| tooltip={button.tooltip} | |
| urlSuffix={button.urlSuffix} | |
| /> | |
| ))} | |
| </div> | |
| </Container> | |
| ); |
| function SidebarButton(props: SidebarButtonProps) { | ||
| const { condition, icon, onClick, selected, title, tooltip, urlSuffix } = | ||
| props; | ||
| const handleOnClick = useCallback(() => { | ||
| if (!selected) { | ||
| onClick(urlSuffix); | ||
| } | ||
| }, [selected, onClick, urlSuffix]); | ||
|
|
||
| return ( | ||
| <Container> | ||
| <Tooltip | ||
| content={tooltip} | ||
| isDisabled={!!title && !tooltip} | ||
| placement={"right"} | ||
| > | ||
| <IconContainer | ||
| className={`t--sidebar-${title || tooltip}`} | ||
| data-selected={selected} | ||
| data-testid={"t--sidebar-" + props.testId} | ||
| onClick={handleOnClick} | ||
| role="button" | ||
| > | ||
| <Icon name={icon} size="lg" /> | ||
| {condition && ( | ||
| <ConditionIcon | ||
| className={`t--sidebar-${condition}-condition-icon`} | ||
| data-testid={`t--sidebar-${condition}-condition-icon`} | ||
| name={ConditionConfig[condition].icon} | ||
| size="md" | ||
| /> | ||
| )} | ||
| </IconContainer> | ||
| </Tooltip> | ||
| {title ? <Text kind="body-s">{title}</Text> : null} | ||
| </Container> | ||
| ); | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add keyboard navigation support.
The button should be keyboard accessible. Consider these improvements:
- Add keyboard event handling
- Include proper ARIA attributes
<IconContainer
className={`t--sidebar-${title || tooltip}`}
data-selected={selected}
data-testid={"t--sidebar-" + props.testId}
onClick={handleOnClick}
+ onKeyDown={(e) => e.key === 'Enter' && handleOnClick()}
+ tabIndex={0}
role="button"
+ aria-pressed={selected}
+ aria-label={tooltip || title}
>📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| function SidebarButton(props: SidebarButtonProps) { | |
| const { condition, icon, onClick, selected, title, tooltip, urlSuffix } = | |
| props; | |
| const handleOnClick = useCallback(() => { | |
| if (!selected) { | |
| onClick(urlSuffix); | |
| } | |
| }, [selected, onClick, urlSuffix]); | |
| return ( | |
| <Container> | |
| <Tooltip | |
| content={tooltip} | |
| isDisabled={!!title && !tooltip} | |
| placement={"right"} | |
| > | |
| <IconContainer | |
| className={`t--sidebar-${title || tooltip}`} | |
| data-selected={selected} | |
| data-testid={"t--sidebar-" + props.testId} | |
| onClick={handleOnClick} | |
| role="button" | |
| > | |
| <Icon name={icon} size="lg" /> | |
| {condition && ( | |
| <ConditionIcon | |
| className={`t--sidebar-${condition}-condition-icon`} | |
| data-testid={`t--sidebar-${condition}-condition-icon`} | |
| name={ConditionConfig[condition].icon} | |
| size="md" | |
| /> | |
| )} | |
| </IconContainer> | |
| </Tooltip> | |
| {title ? <Text kind="body-s">{title}</Text> : null} | |
| </Container> | |
| ); | |
| } | |
| function SidebarButton(props: SidebarButtonProps) { | |
| const { condition, icon, onClick, selected, title, tooltip, urlSuffix } = | |
| props; | |
| const handleOnClick = useCallback(() => { | |
| if (!selected) { | |
| onClick(urlSuffix); | |
| } | |
| }, [selected, onClick, urlSuffix]); | |
| return ( | |
| <Container> | |
| <Tooltip | |
| content={tooltip} | |
| isDisabled={!!title && !tooltip} | |
| placement={"right"} | |
| > | |
| <IconContainer | |
| className={`t--sidebar-${title || tooltip}`} | |
| data-selected={selected} | |
| data-testid={"t--sidebar-" + props.testId} | |
| onClick={handleOnClick} | |
| onKeyDown={(e) => e.key === 'Enter' && handleOnClick()} | |
| tabIndex={0} | |
| role="button" | |
| aria-pressed={selected} | |
| aria-label={tooltip || title} | |
| > | |
| <Icon name={icon} size="lg" /> | |
| {condition && ( | |
| <ConditionIcon | |
| className={`t--sidebar-${condition}-condition-icon`} | |
| data-testid={`t--sidebar-${condition}-condition-icon`} | |
| name={ConditionConfig[condition].icon} | |
| size="md" | |
| /> | |
| )} | |
| </IconContainer> | |
| </Tooltip> | |
| {title ? <Text kind="body-s">{title}</Text> : null} | |
| </Container> | |
| ); | |
| } |
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
app/client/packages/design-system/ads/src/Templates/Sidebar/Sidebar.stories.tsx (3)
12-16: Consider making the template dimensions configurable.The fixed height and width might not represent all usage scenarios. Consider making these configurable through story parameters.
- <Flex background="var(--ads-v2-color-bg)" h="400px" width="100px"> + <Flex + background="var(--ads-v2-color-bg)" + h={args.containerHeight || "400px"} + width={args.containerWidth || "100px"} + >
18-41: Maintain consistent testId naming convention.The testIds use mixed naming conventions:
- "Editor" (PascalCase)
- "warning-button" (kebab-case)
- "settings" (lowercase)
Consider standardizing to kebab-case for all testIds.
- testId: "Editor", + testId: "editor",
43-66: Consider using Storybook actions instead of console.log.Replace console.log with Storybook's actions for better debugging and documentation.
+import { action } from '@storybook/addon-actions'; - onClick: (urlSuffix: string) => console.log("Clicked:", urlSuffix), + onClick: action("sidebar-click"),
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
app/client/packages/design-system/ads/src/Templates/Sidebar/Sidebar.stories.tsx(1 hunks)app/client/packages/design-system/ads/src/Templates/Sidebar/Sidebar.tsx(1 hunks)app/client/src/IDE/Components/Sidebar/index.tsx(0 hunks)
💤 Files with no reviewable changes (1)
- app/client/src/IDE/Components/Sidebar/index.tsx
🚧 Files skipped from review as they are similar to previous changes (1)
- app/client/packages/design-system/ads/src/Templates/Sidebar/Sidebar.tsx
⏰ Context from checks skipped due to timeout of 90000ms (11)
- GitHub Check: perform-test / rts-build / build
- GitHub Check: perform-test / client-build / client-build
- GitHub Check: perform-test / server-build / server-unit-tests
- GitHub Check: client-unit-tests / client-unit-tests
- GitHub Check: client-check-cyclic-deps / check-cyclic-dependencies
- GitHub Check: client-lint / client-lint
- GitHub Check: client-build / client-build
- GitHub Check: client-prettier / prettier-check
- GitHub Check: chromatic-deployment
- GitHub Check: chromatic-deployment
- GitHub Check: storybook-tests
🔇 Additional comments (1)
app/client/packages/design-system/ads/src/Templates/Sidebar/Sidebar.stories.tsx (1)
1-10: LGTM! Clean imports and proper Storybook configuration.
| @@ -19,19 +18,19 @@ const Container = styled(Flex)` | |||
| // the button should be selected and only handle calling the onClick | |||
| export interface IDESidebarButton | |||
| extends Omit<SidebarButtonProps, "onClick" | "selected"> { | |||
| state: EditorState; | |||
There was a problem hiding this comment.
shall we move the styled component to a separate styles file? Also a separate file for types?
There was a problem hiding this comment.
same for the sidebar button component
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (4)
app/client/packages/design-system/ads/src/Templates/Sidebar/styles.ts (1)
5-5: Consider making the width configurable.The fixed width of 50px might not accommodate all use cases. Consider making it configurable through props or CSS variables for better flexibility.
- width: 50px; + width: var(--ads-v2-sidebar-width, 50px);app/client/packages/design-system/ads/src/Templates/Sidebar/SidebarButton/constants.ts (1)
11-13: Create GitHub issues for the pending conditions.The TODO comments indicate missing implementations for Error and Success conditions. Consider creating GitHub issues to track these enhancements.
Would you like me to help create GitHub issues for implementing the Error and Success conditions?
app/client/packages/design-system/ads/src/Templates/Sidebar/SidebarButton/SidebarButton.tsx (2)
24-28: Optimize the click handler dependency array.The
useCallbackdependency array includesselectedwhich causes unnecessary re-renders when selection changes.const handleOnClick = useCallback(() => { if (!selected) { onClick(urlSuffix); } - }, [selected, onClick, urlSuffix]); + }, [onClick, urlSuffix]);
32-36: Add aria-label to Tooltip component.Enhance accessibility by providing an aria-label for the tooltip.
<Tooltip content={tooltip} isDisabled={!!title && !tooltip} placement={"right"} + aria-label={`${title || tooltip} button tooltip`} >
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
app/client/packages/design-system/ads/src/Templates/Sidebar/Sidebar.tsx(2 hunks)app/client/packages/design-system/ads/src/Templates/Sidebar/SidebarButton/SidebarButton.tsx(1 hunks)app/client/packages/design-system/ads/src/Templates/Sidebar/SidebarButton/constants.ts(1 hunks)app/client/packages/design-system/ads/src/Templates/Sidebar/SidebarButton/styles.ts(1 hunks)app/client/packages/design-system/ads/src/Templates/Sidebar/styles.ts(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- app/client/packages/design-system/ads/src/Templates/Sidebar/Sidebar.tsx
⏰ Context from checks skipped due to timeout of 90000ms (11)
- GitHub Check: perform-test / rts-build / build
- GitHub Check: perform-test / server-build / server-unit-tests
- GitHub Check: perform-test / client-build / client-build
- GitHub Check: client-unit-tests / client-unit-tests
- GitHub Check: client-lint / client-lint
- GitHub Check: client-check-cyclic-deps / check-cyclic-dependencies
- GitHub Check: client-build / client-build
- GitHub Check: client-prettier / prettier-check
- GitHub Check: chromatic-deployment
- GitHub Check: chromatic-deployment
- GitHub Check: storybook-tests
🔇 Additional comments (2)
app/client/packages/design-system/ads/src/Templates/Sidebar/styles.ts (1)
6-11: LGTM! Good use of design system variables.Proper usage of design system variables (
--ads-v2-*) for colors and borders ensures consistency across the application.app/client/packages/design-system/ads/src/Templates/Sidebar/SidebarButton/SidebarButton.tsx (1)
37-43: Add keyboard navigation support.The button should be keyboard accessible.
| [Condition.Warn]: { | ||
| icon: "warning", | ||
| color: "#ffe283", | ||
| }, |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Use design system color tokens instead of hardcoded values.
Replace the hardcoded color value with the appropriate design system token for consistency.
[Condition.Warn]: {
icon: "warning",
- color: "#ffe283",
+ color: "var(--ads-v2-color-warning)",
},📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| [Condition.Warn]: { | |
| icon: "warning", | |
| color: "#ffe283", | |
| }, | |
| [Condition.Warn]: { | |
| icon: "warning", | |
| color: "var(--ads-v2-color-warning)", | |
| }, |
| export const IconContainer = styled.div` | ||
| padding: 2px; | ||
| border-radius: 3px; | ||
| width: 32px; | ||
| height: 32px; | ||
| display: flex; | ||
| align-items: center; | ||
| justify-content: center; | ||
| cursor: pointer; | ||
| position: relative; | ||
|
|
||
| &[data-selected="false"] { | ||
| background-color: var(--ads-v2-color-bg); | ||
|
|
||
| &:hover { | ||
| background-color: var(--ads-v2-color-bg-subtle, #f1f5f9); | ||
| } | ||
| } | ||
|
|
||
| &[data-selected="true"] { | ||
| background-color: var(--ads-v2-color-bg-muted); | ||
| } | ||
| `; |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add focus styles for keyboard navigation.
The IconContainer needs focus styles for better keyboard accessibility.
export const IconContainer = styled.div`
/* existing styles */
+ &:focus {
+ outline: 2px solid var(--ads-v2-color-border-focus);
+ outline-offset: 2px;
+ }
+ &:focus:not(:focus-visible) {
+ outline: none;
+ }
`;📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| export const IconContainer = styled.div` | |
| padding: 2px; | |
| border-radius: 3px; | |
| width: 32px; | |
| height: 32px; | |
| display: flex; | |
| align-items: center; | |
| justify-content: center; | |
| cursor: pointer; | |
| position: relative; | |
| &[data-selected="false"] { | |
| background-color: var(--ads-v2-color-bg); | |
| &:hover { | |
| background-color: var(--ads-v2-color-bg-subtle, #f1f5f9); | |
| } | |
| } | |
| &[data-selected="true"] { | |
| background-color: var(--ads-v2-color-bg-muted); | |
| } | |
| `; | |
| export const IconContainer = styled.div` | |
| padding: 2px; | |
| border-radius: 3px; | |
| width: 32px; | |
| height: 32px; | |
| display: flex; | |
| align-items: center; | |
| justify-content: center; | |
| cursor: pointer; | |
| position: relative; | |
| &[data-selected="false"] { | |
| background-color: var(--ads-v2-color-bg); | |
| &:hover { | |
| background-color: var(--ads-v2-color-bg-subtle, #f1f5f9); | |
| } | |
| } | |
| &[data-selected="true"] { | |
| background-color: var(--ads-v2-color-bg-muted); | |
| } | |
| &:focus { | |
| outline: 2px solid var(--ads-v2-color-border-focus); | |
| outline-offset: 2px; | |
| } | |
| &:focus:not(:focus-visible) { | |
| outline: none; | |
| } | |
| `; |
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
app/client/packages/design-system/ads/src/Templates/Sidebar/Sidebar.tsx (1)
10-41: 🛠️ Refactor suggestionImprove accessibility and reduce code duplication.
- Add proper ARIA attributes for navigation
- Extract duplicated button mapping logic
+const renderButtons = (buttons: IDESidebarButton[], onClick: IDESidebarProps['onClick'], editorState: string) => ( + buttons.map((button) => ( + <SidebarButton + key={button.state} + {...button} + onClick={onClick} + selected={editorState === button.state} + /> + )) +); return ( - <Styled.Container className="t--sidebar" id={props.id}> + <Styled.Container + className="t--sidebar" + id={props.id} + role="navigation" + aria-label="IDE Navigation" + > <div> - {topButtons.map((button) => ( - <SidebarButton - condition={button.condition} - icon={button.icon} - key={button.state} - onClick={onClick} - selected={editorState === button.state} - testId={button.testId} - title={button.title} - tooltip={button.tooltip} - urlSuffix={button.urlSuffix} - /> - ))} + {renderButtons(topButtons, onClick, editorState)} </div> <div> - {bottomButtons.map((button) => ( - <SidebarButton - condition={button.condition} - icon={button.icon} - key={button.state} - onClick={onClick} - selected={editorState === button.state} - testId={button.testId} - title={button.title} - tooltip={button.tooltip} - urlSuffix={button.urlSuffix} - /> - ))} + {renderButtons(bottomButtons, onClick, editorState)} </div> </Styled.Container> );
🧹 Nitpick comments (3)
app/client/packages/design-system/ads/src/Templates/Sidebar/SidebarButton/SidebarButton.types.ts (1)
3-12: Enhance type safety and accessibility support in SidebarButtonProps.
- The
iconprop should have a more specific type thanstringto ensure valid icons are passed.- Consider adding accessibility-related props.
export interface SidebarButtonProps { title?: string; testId: string; selected: boolean; - icon: string; + icon: IconName; // Create a union type of valid icon names onClick: (urlSuffix: string) => void; urlSuffix: string; tooltip?: string; condition?: Condition; + ariaLabel?: string; }app/client/packages/design-system/ads/src/Templates/Sidebar/types.ts (1)
5-9: Consider type safety for state values.The
stateproperty should be constrained to valid editor states to prevent runtime errors.+type EditorState = 'edit' | 'preview' | 'deploy'; // Add valid states export interface IDESidebarButton extends Omit<SidebarButtonProps, "onClick" | "selected"> { - state: string; + state: EditorState; urlSuffix: string; }app/client/packages/design-system/ads/src/Templates/Sidebar/Sidebar.tsx (1)
6-8: Add memoization for performance optimization.Consider memoizing the component to prevent unnecessary re-renders.
-export function IDESidebar(props: IDESidebarProps) { +export const IDESidebar = React.memo(function IDESidebar(props: IDESidebarProps) { const { bottomButtons, editorState, onClick, topButtons } = props;
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (8)
app/client/packages/design-system/ads/src/Templates/Sidebar/Sidebar.stories.tsx(1 hunks)app/client/packages/design-system/ads/src/Templates/Sidebar/Sidebar.tsx(1 hunks)app/client/packages/design-system/ads/src/Templates/Sidebar/SidebarButton/SidebarButton.test.tsx(4 hunks)app/client/packages/design-system/ads/src/Templates/Sidebar/SidebarButton/SidebarButton.tsx(1 hunks)app/client/packages/design-system/ads/src/Templates/Sidebar/SidebarButton/SidebarButton.types.ts(1 hunks)app/client/packages/design-system/ads/src/Templates/Sidebar/SidebarButton/index.ts(1 hunks)app/client/packages/design-system/ads/src/Templates/Sidebar/index.tsx(1 hunks)app/client/packages/design-system/ads/src/Templates/Sidebar/types.ts(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (5)
- app/client/packages/design-system/ads/src/Templates/Sidebar/SidebarButton/index.ts
- app/client/packages/design-system/ads/src/Templates/Sidebar/index.tsx
- app/client/packages/design-system/ads/src/Templates/Sidebar/Sidebar.stories.tsx
- app/client/packages/design-system/ads/src/Templates/Sidebar/SidebarButton/SidebarButton.tsx
- app/client/packages/design-system/ads/src/Templates/Sidebar/SidebarButton/SidebarButton.test.tsx
⏰ Context from checks skipped due to timeout of 90000ms (9)
- GitHub Check: perform-test / rts-build / build
- GitHub Check: client-unit-tests / client-unit-tests
- GitHub Check: client-lint / client-lint
- GitHub Check: client-check-cyclic-deps / check-cyclic-dependencies
- GitHub Check: client-prettier / prettier-check
- GitHub Check: client-build / client-build
- GitHub Check: chromatic-deployment
- GitHub Check: chromatic-deployment
- GitHub Check: storybook-tests
Description
We are making the IDE sidebar an ADS template that can be used by other IDEs with ease
Fixes #38640
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/12766796609
Commit: 5466245
Cypress dashboard.
Tags:
@tag.IDESpec:
Tue, 14 Jan 2025 12:10:17 UTC
Communication
Should the DevRel and Marketing teams inform users about this change?
Summary by CodeRabbit
New Features
IDESidebarcomponent for enhanced sidebar functionality.SidebarButtoncomponent with interactive elements and conditional rendering.Conditionenum for button state management.Sidebarmodule.Bug Fixes
IDESidebarButtonandIDESidebarPropsfor improved compatibility.Documentation
IDESidebarcomponent to showcase usage examples.Refactor