chore: Move IDE header to ADS/Templates #37406
Conversation
WalkthroughThe changes in this pull request involve the restructuring and migration of the IDE Header component and its related elements into the Appsmith Design System (ADS). This includes the addition of new components, updates to existing exports, and the removal of obsolete files. The refactor aims to enhance the organization of the header components and improve their accessibility within the design system. 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
the
skip-changelog
hetunandu
changed the title
hetunandu
changed the title
hetunandu
added
ok-to-test
github-actions
bot
added
IDE Navigation
|
/build-deploy-preview |
|
Deploying Your Preview: https://github.com/appsmithorg/appsmith/actions/runs/11947739097. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 8
🧹 Outside diff range and nitpick comments (15)
app/client/packages/design-system/ads/src/Layout/IDEHeader/IDEHeaderTitle.tsx (1)
Line range hint
5-16: LGTM! Consider enhancing the documentation.The implementation looks clean and follows best practices. Consider adding JSDoc tags for better documentation:
/** * Handy little styled component that can be used to render the title in the IDEHeader component + * @param {Object} props - Component props + * @param {string} props.title - The title text to display + * @returns {JSX.Element} A flex container with centered title text **/app/client/packages/design-system/ads/src/Layout/IDEHeader/HeaderSwitcher/HeaderSwitcher.styles.ts (2)
4-15: Consider enhancing accessibility and user experience.The component should include:
- Focus styles for keyboard navigation
- Visual distinction between hover and active states
- Smooth transitions between states
export const SwitchTrigger = styled.div<{ active: boolean }>` display: flex; border-radius: var(--ads-v2-border-radius); background-color: ${(props) => props.active ? `var(--ads-v2-color-bg-subtle)` : "unset"}; cursor: pointer; padding: var(--ads-v2-spaces-2); + transition: all 0.2s ease; + + &:focus-visible { + outline: 2px solid var(--ads-v2-color-border-emphasis); + outline-offset: -2px; + } :hover { - background-color: var(--ads-v2-color-bg-subtle); + background-color: ${(props) => + props.active + ? `var(--ads-v2-color-bg-subtle)` + : `var(--ads-v2-color-bg-muted)`}; } `;
17-20: Use design system spacing variables for consistency.Replace the hardcoded em value with the design system spacing variable.
export const ContentContainer = styled(PopoverContent)` padding: 0; - padding-bottom: 0.25em; + padding-bottom: var(--ads-v2-spaces-1); `;app/client/packages/design-system/ads/src/Layout/EntityExplorer/styles.ts (1)
7-8: Consider making heights configurableBoth components use hardcoded heights (32px and 40px). Consider using CSS variables for these dimensions to maintain consistency and flexibility across the design system.
- height: 32px; + height: var(--ads-v2-unit-4); - height: 40px; + height: var(--ads-v2-unit-5);Also applies to: 21-22
app/client/packages/design-system/ads/src/Layout/IDEHeader/index.ts (1)
10-14: Consider exporting the dropdown item type.The IDEHeaderSwitcher documentation suggests it handles a list of elements. Consider exporting its item type for better developer experience.
+/** + * Type for items that can be used in the IDEHeaderSwitcher dropdown + */ +export type { IDEHeaderSwitcherItem } from "./HeaderSwitcher";app/client/packages/design-system/ads/src/Layout/IDEHeader/IDEHeader.tsx (3)
5-7: Consider simplifying the children type definitionThe union type
React.ReactNode | React.ReactNode[]is redundant sinceReact.ReactNodealready includes arrays.interface ChildrenProps { - children: React.ReactNode | React.ReactNode[]; + children: React.ReactNode; }
9-31: LGTM! Consider extracting logo container stylesThe implementation is clean and well-structured. Consider extracting the logo container styles into a styled component for better reusability.
const LogoContainer = styled(Flex)` align-items: center; border-right: 1px solid var(--ads-v2-color-border); height: 100%; justify-content: center; width: ${LOGO_WIDTH}px; `;
62-76: Consider performance optimization and height handling improvementsThe implementation is solid but could benefit from two improvements:
- Template literal for height value
- Memoization for performance
-export const IDEHeader = (props: ChildrenProps) => { +export const IDEHeader = React.memo((props: ChildrenProps) => { return ( <Flex alignItems="center" background="var(--ads-v2-color-bg)" borderBottom="1px solid var(--ads-v2-color-border)" className="t--editor-header" - height={IDE_HEADER_HEIGHT + "px"} + height={`${IDE_HEADER_HEIGHT}px`} overflow="hidden" width="100%" > {props.children} </Flex> ); -}; +});app/client/packages/design-system/ads/src/Layout/IDEHeader/HeaderSwitcher/IDEHeaderSwitcher.tsx (2)
8-17: Add JSDoc comments to document the Props interface.Adding documentation will improve maintainability and help other developers understand the purpose of each prop.
+/** + * Props for the IDEHeaderSwitcher component + * @property {string} prefix - The prefix text to display before the title + * @property {string} [title] - Optional title text + * @property {string} titleTestId - Test ID for the title element + * @property {boolean} active - Whether the switcher is in active state + * @property {(active: boolean) => void} setActive - Callback to update active state + * @property {React.MouseEventHandler<HTMLDivElement>} [onClick] - Optional click handler + * @property {string} [className] - Optional CSS class name + * @property {React.ReactNode} children - Content to render in the popover + */ interface Props {
68-76: Simplify the icon color logic.The conditional color assignment can be simplified using the nullish coalescing operator.
<Icon - color={ - title - ? undefined - : "var(--ads-v2-colors-content-label-inactive-fg)" - } + color={title ?? "var(--ads-v2-colors-content-label-inactive-fg)"} name={active ? "arrow-up-s-line" : "arrow-down-s-line"} size="md" />app/client/packages/design-system/ads/src/Layout/IDEHeader/IDEHeader.stories.tsx (1)
20-24: Simplify the decorator implementationThe decorator can be simplified using an inline style object.
decorators: [ - (Story: () => React.ReactNode) => ( - <div style={{ width: "100%" }}>{Story()}</div> - ), + (Story) => <div style={{ width: "100%" }}>{Story()}</div>, ],app/client/src/pages/Editor/IDE/EditorPane/PagesSection.tsx (2)
70-75: Consider making the maxHeight responsive or configurableThe hardcoded maxHeight of 300px might not be optimal for all screen sizes and resolutions.
Consider using:
- maxHeight={"300px"} + maxHeight={{ base: "300px", lg: "400px" }}
Line range hint
70-101: Well-structured migration to ADS componentsThe refactoring successfully moves the component to use ADS while maintaining a clean and maintainable structure. The use of Flex and List components provides a good foundation for future extensions.
Consider extracting the layout configuration into a theme constant if these values (maxHeight, spacing) will be reused across similar components in the IDE.
app/client/packages/design-system/ads/src/Layout/IDEHeader/HeaderSwitcher/HeaderSwitcher.test.tsx (2)
8-16: Consider extracting test data to a separate fixture fileThe test data in defaultProps could be moved to a separate fixture file to improve maintainability and reusability across different test files.
+ // test-fixtures/headerSwitcher.ts + export const mockData = { + prefix: "Prefix", + title: "Title", + titleTestId: "titleTestId", + active: false, + children: <span>Test</span>, + };
35-37: LGTM! Consider adding more style assertionsThe active state test is good but could be more comprehensive by checking additional style properties that change with the active state.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (31)
app/client/packages/design-system/ads/src/Layout/EntityExplorer/index.ts(1 hunks)app/client/packages/design-system/ads/src/Layout/EntityExplorer/styles.ts(1 hunks)app/client/packages/design-system/ads/src/Layout/IDEHeader/HeaderSwitcher/HeaderSwitcher.styles.ts(1 hunks)app/client/packages/design-system/ads/src/Layout/IDEHeader/HeaderSwitcher/HeaderSwitcher.test.tsx(2 hunks)app/client/packages/design-system/ads/src/Layout/IDEHeader/HeaderSwitcher/IDEHeaderSwitcher.tsx(1 hunks)app/client/packages/design-system/ads/src/Layout/IDEHeader/HeaderSwitcher/index.ts(1 hunks)app/client/packages/design-system/ads/src/Layout/IDEHeader/IDEHeader.constants.ts(1 hunks)app/client/packages/design-system/ads/src/Layout/IDEHeader/IDEHeader.mdx(1 hunks)app/client/packages/design-system/ads/src/Layout/IDEHeader/IDEHeader.stories.tsx(1 hunks)app/client/packages/design-system/ads/src/Layout/IDEHeader/IDEHeader.tsx(1 hunks)app/client/packages/design-system/ads/src/Layout/IDEHeader/IDEHeaderTitle.tsx(1 hunks)app/client/packages/design-system/ads/src/Layout/IDEHeader/index.ts(1 hunks)app/client/packages/design-system/ads/src/Layout/index.ts(1 hunks)app/client/packages/design-system/ads/src/index.ts(1 hunks)app/client/src/IDE/Components/HeaderDropdown.test.tsx(0 hunks)app/client/src/IDE/Components/HeaderDropdown.tsx(0 hunks)app/client/src/IDE/Components/HeaderEditorSwitcher.tsx(0 hunks)app/client/src/IDE/index.ts(0 hunks)app/client/src/ce/constants/messages.ts(1 hunks)app/client/src/pages/AdminSettings/components.tsx(1 hunks)app/client/src/pages/AppViewer/Navigation/Sidebar.tsx(1 hunks)app/client/src/pages/Editor/EditorName/components.ts(1 hunks)app/client/src/pages/Editor/IDE/EditorPane/PagesSection.tsx(3 hunks)app/client/src/pages/Editor/IDE/Header/EditorTitle.tsx(1 hunks)app/client/src/pages/Editor/IDE/Header/index.tsx(3 hunks)app/client/src/pages/Editor/JSEditor/styledComponents.ts(1 hunks)app/client/src/pages/Editor/WidgetsEditor/WidgetEditorContainer.tsx(1 hunks)app/client/src/pages/Editor/commons/EditorHeaderComponents.tsx(1 hunks)app/client/src/pages/Editor/commons/EditorWrapperContainer.tsx(1 hunks)app/client/src/pages/Editor/commons/Omnibar.tsx(1 hunks)app/client/src/pages/Editor/index.tsx(1 hunks)
💤 Files with no reviewable changes (4)
- app/client/src/IDE/Components/HeaderDropdown.test.tsx
- app/client/src/IDE/Components/HeaderDropdown.tsx
- app/client/src/IDE/Components/HeaderEditorSwitcher.tsx
- app/client/src/IDE/index.ts
✅ Files skipped from review due to trivial changes (11)
- app/client/packages/design-system/ads/src/Layout/EntityExplorer/index.ts
- app/client/packages/design-system/ads/src/Layout/IDEHeader/HeaderSwitcher/index.ts
- app/client/packages/design-system/ads/src/Layout/IDEHeader/IDEHeader.constants.ts
- app/client/packages/design-system/ads/src/Layout/IDEHeader/IDEHeader.mdx
- app/client/packages/design-system/ads/src/Layout/index.ts
- app/client/src/pages/AdminSettings/components.tsx
- app/client/src/pages/AppViewer/Navigation/Sidebar.tsx
- app/client/src/pages/Editor/EditorName/components.ts
- app/client/src/pages/Editor/WidgetsEditor/WidgetEditorContainer.tsx
- app/client/src/pages/Editor/commons/EditorWrapperContainer.tsx
- app/client/src/pages/Editor/index.tsx
🔇 Additional comments (26)
app/client/packages/design-system/ads/src/Layout/IDEHeader/IDEHeaderTitle.tsx (1)
Line range hint 8-16: Verify integration with other IDE header components.
Let's ensure this component is properly integrated with other IDE header components after the move to ADS.
✅ Verification successful
Component is properly integrated with IDE header components
The IDEHeaderTitle component is correctly integrated within the IDE header ecosystem:
- Used appropriately in the main Header component for different editor states
- Properly imported and exported through the IDE header module
- Demonstrated correct usage in storybook examples
- Follows the layout composition pattern with IDEHeader.Left/Center/Right sections
The implementation aligns well with the design system's patterns and shows no integration issues.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check usage and integration of IDEHeaderTitle component
# Check for imports of IDEHeaderTitle
rg -l "IDEHeaderTitle" --type ts --type tsx
# Check for usage patterns
ast-grep --pattern 'IDEHeaderTitle title={$_}'
Length of output: 120
Script:
#!/bin/bash
# Let's try a different approach to check integration
# Search for IDEHeaderTitle in all files (without file type restriction)
rg "IDEHeaderTitle"
# Find TypeScript/React files that might contain IDE header components
fd -e ts -e tsx . app/client/packages/design-system/ads/src/Layout/IDEHeader/
# Check content of the IDEHeader directory
ls -la app/client/packages/design-system/ads/src/Layout/IDEHeader/
Length of output: 2515
Script:
#!/bin/bash
# Let's check the actual implementation and usage patterns
# Check IDEHeader implementation to understand integration
cat app/client/packages/design-system/ads/src/Layout/IDEHeader/IDEHeader.tsx
# Check actual usage in the Editor
cat app/client/src/pages/Editor/IDE/Header/index.tsx
# Check stories for usage examples
cat app/client/packages/design-system/ads/src/Layout/IDEHeader/IDEHeader.stories.tsx
Length of output: 17351
app/client/packages/design-system/ads/src/Layout/IDEHeader/HeaderSwitcher/HeaderSwitcher.styles.ts (1)
1-2: LGTM! Clean imports.
app/client/packages/design-system/ads/src/Layout/EntityExplorer/styles.ts (3)
1-2: LGTM: Clean imports
Imports are clean and necessary for the component implementation.
16-20: LGTM: Well-structured header layout
Good use of flex layout and CSS variables for spacing. The line-height on span elements ensures consistent text alignment.
Also applies to: 23-25
6-6: Review grid template columns definition
The grid template with 8 columns starting with width 0 seems unusual. Consider explicitly defining column widths or using fr units for better maintainability.
- grid-template-columns: 0 auto 1fr auto auto auto auto auto;
+ grid-template-columns: min-content auto 1fr repeat(5, auto);✅ Verification successful
Grid template columns pattern is consistent with IDE implementation
The grid-template-columns: 0 auto 1fr auto auto auto auto auto pattern is actually used consistently across IDE-related components in the codebase:
app/client/src/pages/Editor/IDE/EditorPane/JS/List.tsxapp/client/src/pages/Editor/IDE/EditorPane/Query/Explorer.tsxapp/client/src/pages/Editor/IDE/EditorTabs/List.tsx
This is an established pattern for IDE layouts in the application, so the implementation is correct.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for similar grid patterns in the codebase to verify if this is a consistent pattern
rg -l "grid-template-columns:.+0\s+auto" --type css --type typescript
Length of output: 106
Script:
#!/bin/bash
# Search for similar grid patterns in the codebase with correct file type flags
rg -l "grid-template-columns:\s*0\s+auto" -t ts -t tsx -t css
# Also search for the specific component usage to understand the grid structure
rg -A 5 "ListItemContainer" -t ts -t tsx
# Look for similar styled components with grid layouts
ast-grep --pattern 'styled($$$).attrs($_)? {
$$$
grid-template-columns: $$$;
$$$
}'
Length of output: 252
Script:
#!/bin/bash
# Search for grid patterns in all files
rg "grid-template-columns" --type-add 'ts:*.{ts,tsx}' -t ts
# Find ListItemContainer usage
rg "ListItemContainer" --type-add 'ts:*.{ts,tsx}' -t ts -A 5
# Find the component file to understand its structure
fd "EntityExplorer" --type f --exec cat {}
Length of output: 51769
app/client/src/pages/Editor/IDE/Header/EditorTitle.tsx (3)
1-2: LGTM! Clean import structure
The imports are well-organized and align with the migration to ADS/Templates.
10-12: Proper callback optimization
Good use of useCallback for the menu close handler with correct dependency array.
15-23: Clean implementation of IDEHeaderSwitcher
The component structure follows ADS patterns correctly with proper prop passing.
Let's verify the consistency of the testId pattern:
✅ Verification successful
testId pattern follows design system conventions
The t--pages-switcher testId is correctly implemented and consistent with the codebase patterns. The IDEHeaderSwitcher component is a part of the design system and the test ID is properly used in both the component and its corresponding Cypress tests.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for similar testId patterns to ensure consistency
rg "t--.*-switcher" --type ts --type tsx
Length of output: 70
Script:
#!/bin/bash
# Search for similar testId patterns without file type restrictions
rg "t--.*-switcher"
# Also search for general testId patterns to understand the convention
rg "testId=\"t--" -A 1
Length of output: 414
Script:
#!/bin/bash
# Search for all test-related attributes in the codebase to understand the naming pattern
rg "data-testid|data-test-id|testId|test-id" -A 1
# Also check the cypress test file to see how the switcher is used
cat "app/client/cypress/support/Pages/PageList.ts"
Length of output: 110756
Script:
#!/bin/bash
# Let's check the test file to understand how the switcher is used
cat "app/client/cypress/support/Pages/PageList.ts"
# Also search for similar switcher components to verify the pattern
rg -A 2 "IDEHeaderSwitcher" --type ts --type tsx
Length of output: 4089
Script:
#!/bin/bash
# Search for IDEHeaderSwitcher without file type restrictions
rg "IDEHeaderSwitcher" -A 2
# Also search for the test ID pattern in the component
rg "t--pages-switcher"
Length of output: 6872
app/client/packages/design-system/ads/src/Layout/IDEHeader/index.ts (2)
16-19: LGTM!
The IDEHeaderTitle export is well-documented with clear usage context.
1-8: LGTM! Verify the exported constants.
The IDEHeader exports and documentation are well-structured. Let's verify the constants are properly defined.
✅ Verification successful
Constants are properly defined and exported
The constants IDE_HEADER_HEIGHT and LOGO_WIDTH are correctly defined in IDEHeader.constants.ts with values 40 and 50 respectively. The exports match the barrel file imports.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify the constants are properly defined and exported
ast-grep --pattern 'export const IDE_HEADER_HEIGHT = $_'
ast-grep --pattern 'export const LOGO_WIDTH = $_'
Length of output: 142238
Script:
#!/bin/bash
# Let's try a different approach to find the constants by searching in the IDEHeader.constants.ts file
rg -U "IDE_HEADER_HEIGHT|LOGO_WIDTH" app/client/packages/design-system/ads/src/Layout/IDEHeader/IDEHeader.constants.ts
Length of output: 187
app/client/packages/design-system/ads/src/index.ts (1)
39-39: LGTM! Export follows consistent pattern
The new Layout export follows the established barrel file pattern and maintains alphabetical ordering.
app/client/src/pages/Editor/commons/Omnibar.tsx (1)
6-6: LGTM! Import change aligns with ADS migration.
The update to import IDE_HEADER_HEIGHT from @appsmith/ads is consistent with the PR objective of moving IDE components to ADS/Templates.
Let's verify the consistency of IDE_HEADER_HEIGHT usage:
✅ Verification successful
Import change is consistent with ADS migration
The constant IDE_HEADER_HEIGHT is correctly imported from @appsmith/ads across all consuming components, and its source is properly defined in the design system package at app/client/packages/design-system/ads/src/Layout/IDEHeader/.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify IDE_HEADER_HEIGHT imports and usage across the codebase
# Expected: All imports should be from @appsmith/ads
# Check for any remaining imports from old location
rg "IDE_HEADER_HEIGHT.*from.*IDE"
# Check all current imports to ensure consistency
rg "IDE_HEADER_HEIGHT.*from.*@appsmith/ads"
Length of output: 1335
app/client/packages/design-system/ads/src/Layout/IDEHeader/IDEHeader.tsx (3)
33-45: LGTM! Clean implementation
The Center component follows consistent patterns and has appropriate flex properties.
47-60: LGTM! Consistent implementation
The Right component maintains consistency with other sections while appropriately handling right alignment.
78-80: LGTM! Well-implemented compound component pattern
The static assignment of subcomponents follows React best practices for compound components.
app/client/src/pages/Editor/JSEditor/styledComponents.ts (2)
Line range hint 35-42: Verify constant value consistency
The FormWrapper's height calculation depends on this constant. Ensure the value remains consistent after the migration.
✅ Verification successful
IDE_HEADER_HEIGHT constant value is consistent
The constant is defined with the same value of 40px in both locations:
app/client/src/IDE/Structure/constants.tsapp/client/packages/design-system/ads/src/Layout/IDEHeader/IDEHeader.constants.ts
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check for any value changes in the constant definition
rg -p "IDE_HEADER_HEIGHT.*=.*\d+" --type ts
Length of output: 248
9-9: Verify IDE_HEADER_HEIGHT constant in new location
The import path change aligns with the IDE Header migration to ADS. Let's verify the constant's existence and consistent usage.
✅ Verification successful
Import change is correct and consistent
The constant IDE_HEADER_HEIGHT is properly defined in the new location at @appsmith/ads and all imports across the codebase are using the new path, except for internal references within the ADS package itself. The constant maintains its value of 40px and is correctly used in the FormWrapper component for layout calculations.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Verify the constant exists in the new location and check for any inconsistent imports
echo "Checking for IDE_HEADER_HEIGHT constant definition:"
rg -p "export.*IDE_HEADER_HEIGHT" --type ts
echo "\nChecking for any remaining old imports:"
rg -p "import.*IDE_HEADER_HEIGHT.*from.*\"IDE\"" --type ts
Length of output: 655
Script:
#!/bin/bash
# Check for all imports of IDE_HEADER_HEIGHT to ensure consistency
echo "Checking all IDE_HEADER_HEIGHT imports:"
rg -p "import.*IDE_HEADER_HEIGHT.*from" --type ts
# Check usage of IDE_HEADER_HEIGHT in the file under review
echo "\nChecking usage in the file under review:"
cat "app/client/src/pages/Editor/JSEditor/styledComponents.ts"
Length of output: 4335
app/client/packages/design-system/ads/src/Layout/IDEHeader/HeaderSwitcher/IDEHeaderSwitcher.tsx (3)
19-86: LGTM! Well-structured component with good practices.
The component follows React best practices with proper ref forwarding, keyboard accessibility, and clean prop handling.
88-88: LGTM! Proper display name assignment.
Setting the display name helps with debugging and dev tools identification.
1-88: Verify ADS integration points.
Let's ensure this component is properly integrated into the ADS system.
✅ Verification successful
Component is properly integrated within ADS
The verification shows correct export chain through index files and proper import usage:
- Exported correctly through
Layout/IDEHeader/HeaderSwitcher/index.tsandLayout/IDEHeader/index.ts - Imported correctly in the IDE editor via
@appsmith/ads - Used appropriately in stories and tests
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check exports and imports in the ADS system
echo "Checking exports..."
rg -g '*.ts' -g '*.tsx' "export.*IDEHeaderSwitcher" ./app/client/packages/design-system/ads/src/
echo "Checking imports..."
rg -g '*.ts' -g '*.tsx' "import.*IDEHeaderSwitcher" ./app
Length of output: 1073
app/client/src/pages/Editor/IDE/EditorPane/PagesSection.tsx (2)
2-7: LGTM! Clean import of ADS components
The new imports from @appsmith/ads align well with the PR's objective of moving to the Application Development System.
89-100: Verify the design system token and overflow behavior
The layout structure looks good, but let's verify two aspects:
- The
spaces-2token usage - The overflow behavior with many pages
✅ Verification successful
Let me analyze the spacing token usage and overflow patterns to provide a more complete verification. The previous search didn't give us enough context about overflow patterns, so let's try a different approach.
Design tokens and overflow behavior are correctly implemented
The spaces-2 token (4px) is a valid design system token as shown in the Flex component stories, and the overflow behavior follows the established pattern used in similar list containers in the codebase.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check if spaces-2 token is defined in the design system
rg -g '*.{ts,tsx,js,jsx}' '"spaces-2"' --type-add 'tsx:*.{ts,tsx}' --type tsx
# Find similar components to verify overflow handling patterns
ast-grep --pattern 'overflow="auto"'
Length of output: 148263
Script:
#!/bin/bash
# Check design system spacing tokens
rg -g '*.{ts,tsx}' 'spaces-[0-9]' app/client/packages/design-system
# Find similar overflow patterns in list/container components
ast-grep --pattern 'overflow="auto"'
Length of output: 100142
Script:
#!/bin/bash
# Look for similar list container components with overflow handling
rg -g '*.{ts,tsx}' -B 2 -A 2 'overflow.*auto.*Flex|Flex.*overflow.*auto' app/client/src/
Length of output: 1008
app/client/src/pages/Editor/IDE/Header/index.tsx (3)
16-17: LGTM: Import changes align with ADS migration
The addition of IDEHeader and IDEHeaderTitle imports from @appsmith/ads, along with AppsmithLink, correctly implements the migration to ADS/Templates.
Also applies to: 82-82
Line range hint 271-350: LGTM: Modal and publishing functionality preserved
The modal handling, publishing logic, and Git integration checks are correctly maintained within the new header structure.
Line range hint 223-227: Verify header layout maintains responsiveness
The new IDEHeader structure with Left/Center/Right sections improves organization while maintaining functionality. The use of slots pattern aligns with ADS templates design.
Let's verify no other components are still importing the old IDE header:
✅ Verification successful
IDE Header imports are properly maintained across the codebase
The new IDE Header structure is correctly referenced in the codebase. All imports are aligned with the new component structure:
- Main header component is imported in
ce/pages/common/AppHeader.tsx - Utility hooks are properly imported through the EE/CE chain
- No legacy imports found that would conflict with the new structure
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for any remaining imports of the old IDE header
rg -l "from.*IDE/Header" --type ts --type tsx
Length of output: 75
Script:
#!/bin/bash
# Search for any remaining imports of the old IDE header using ripgrep with correct file type
rg -l "from.*IDE/Header" -t typescript -t tsx
# Let's also check for any direct references to IDE/Header
rg -l "IDE/Header" -t typescript -t tsx
# Search for potential IDE header imports with ast-grep
ast-grep --pattern 'import { $_ } from "$_/IDE/Header$_"'
Length of output: 217
Script:
#!/bin/bash
# Search for IDE header imports and references without file type restrictions
rg "from.*IDE/Header"
rg "IDE/Header"
# Let's also check for any files in the IDE/Header directory
fd Header app/client/src/pages/Editor/IDE
Length of output: 1428
app/client/src/ce/constants/messages.ts (1)
2478-2479: Verify header title change impact
The header title change from "Editor" to "Pages" looks good, but we should ensure all consuming components are updated.
✅ Verification successful
Let me generate new shell scripts to verify the header title changes and their impact:
This will help verify:
- All usages of HEADER_TITLES constants across the codebase
- Any references to "Editor" or "Pages" specifically in header-related files
The previous search didn't provide enough context about the header title usage. These new commands should give us better visibility into the impact of the changes.
Header title change is properly implemented across the codebase
The change from "Editor" to "Pages" in HEADER_TITLES is correctly reflected in all consuming components:
- The constant is properly defined in messages.ts
- EditorTitle.tsx uses HEADER_TITLES.PAGES correctly
- No lingering references to HEADER_TITLES.EDITOR were found
- Other header titles (DATA, SETTINGS, LIBRARIES) remain unaffected
The change appears to be complete and consistent across the codebase.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for any remaining references to the old header title
rg -A 2 "HEADER_TITLES.EDITOR"
# Search for components that may need updates
ast-grep --pattern 'HEADER_TITLES.$_'
Length of output: 151733
Script:
#!/bin/bash
# Search for HEADER_TITLES usage in components
rg -A 2 "HEADER_TITLES" app/client/src/
# Search for any references to Editor or Pages in header related files
rg -A 2 "Editor|Pages" app/client/src/pages/Editor/IDE/Header/
Length of output: 8258
app/client/packages/design-system/ads/src/Layout/IDEHeader/IDEHeader.stories.tsx
Outdated
Show resolved
Hide resolved
app/client/packages/design-system/ads/src/Layout/IDEHeader/IDEHeader.stories.tsx
Outdated
Show resolved
Hide resolved
app/client/packages/design-system/ads/src/Layout/IDEHeader/IDEHeader.stories.tsx
Outdated
Show resolved
Hide resolved
...lient/packages/design-system/ads/src/Layout/IDEHeader/HeaderSwitcher/HeaderSwitcher.test.tsx
Outdated
Show resolved
Hide resolved
...lient/packages/design-system/ads/src/Layout/IDEHeader/HeaderSwitcher/HeaderSwitcher.test.tsx
Outdated
Show resolved
Hide resolved
...lient/packages/design-system/ads/src/Layout/IDEHeader/HeaderSwitcher/HeaderSwitcher.test.tsx
Outdated
Show resolved
Hide resolved
|
/build-deploy-preview |
|
Deploying Your Preview: https://github.com/appsmithorg/appsmith/actions/runs/11948518162. |
| import { Flex } from "../../Flex"; | ||
|
|
||
| export const ListItemContainer = styled(Flex)` | ||
| & .t--entity-item { |
There was a problem hiding this comment.
You're probably just moving, but applying styling through hardcoded selectors to children is an anti-pattern. The way I see it - such styling should not exist in ADS.
There was a problem hiding this comment.
Yes, this is just a move. I have created this into EntityExplorer because this needs to be updated soon. These classes will soon be part of the EntityExplorer template itself.
app/client/packages/design-system/ads/src/Templates/EntityExplorer/styles.ts
Show resolved
Hide resolved
app/client/packages/design-system/ads/src/Templates/IDEHeader/IDEHeader.tsx
Outdated
Show resolved
Hide resolved
| ); | ||
| }; | ||
|
|
||
| IDEHeader.Left = Left; |
There was a problem hiding this comment.
This is a confusing way to mutate and export components, is there a reason for this?
There was a problem hiding this comment.
I wanted to namespace the components in a way to ensure these sub areas are used with the main "IDEHeader" component usage . Another option is to export components named "IDEHeaderLeft" which might just get too verbose but I will defer to your judgement here
There was a problem hiding this comment.
You could just export them as usual, from the index (because they should be separate components) and import with alias.
// export from IDEHeader/index.ts
export { Left } from './Left';
export { Center } from './Center';
export { Right } from './Right';
// import wherever it needs to be used
import { * as IDEHeader } from './IDEHeader'
// render
return <IDEHeader.Left />
|
/build-deploy-preview |
|
Deploying Your Preview: https://github.com/appsmithorg/appsmith/actions/runs/11951374629. |
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Outside diff range and nitpick comments (19)
app/client/packages/design-system/ads/src/Templates/IDEHeader/IDEHeaderTitle.tsx (1)
Line range hint
8-17: Consider extracting props interface for better reusabilityThe component implementation looks good, but we could improve type reusability.
Consider this enhancement:
+interface IDEHeaderTitleProps { + title: string; +} + -export const IDEHeaderTitle = ({ title }: { title: string }) => { +export const IDEHeaderTitle = ({ title }: IDEHeaderTitleProps) => {app/client/packages/design-system/ads/src/Templates/IDEHeader/HeaderSwitcher/HeaderSwitcher.styles.ts (2)
4-15: Consider adding transition for smoother state changesThe component looks good but could benefit from a smoother user experience.
Consider adding a transition for background-color changes:
export const SwitchTrigger = styled.div<{ active: boolean }>` display: flex; border-radius: var(--ads-v2-border-radius); background-color: ${(props) => props.active ? `var(--ads-v2-color-bg-subtle)` : "unset"}; cursor: pointer; padding: var(--ads-v2-spaces-2); + transition: background-color 0.2s ease; :hover { background-color: var(--ads-v2-color-bg-subtle); } `;
17-20: Maintain consistency in CSS unitsConsider using CSS variables for padding to maintain consistency with the design system.
export const ContentContainer = styled(PopoverContent)` padding: 0; - padding-bottom: 0.25em; + padding-bottom: var(--ads-v2-spaces-1); `;app/client/packages/design-system/ads/src/Templates/EntityExplorer/styles.ts (2)
4-13: Consider improving the component structureWhile this is a temporary move, there are several areas that could be improved:
- The grid template columns use magic numbers - consider using named template areas or CSS variables
- The hardcoded height value could be moved to a design token
- Deep nesting with class selectors could make maintenance difficult
+// TODO: Replace with proper templating when updating EntityExplorer export const ListItemContainer = styled(Flex)` & .t--entity-item { - grid-template-columns: 0 auto 1fr auto auto auto auto auto; + grid-template-columns: var(--explorer-grid-template); - height: 32px; + height: var(--ads-v2-spaces-8); & .t--entity-name { padding-left: var(--ads-v2-spaces-3); } } `;
15-26: Consider standardizing dimensionsThe component could benefit from:
- Using a consistent height token (IDE_HEADER_HEIGHT as suggested in previous review)
- Standardizing padding across all sides using the same spacing token
export const ListHeaderContainer = styled.div` padding: var(--ads-v2-spaces-3); - padding-right: var(--ads-v2-spaces-2); + padding-right: var(--ads-v2-spaces-3); display: flex; justify-content: space-between; align-items: center; - height: 40px; + height: var(--ide-header-height); span { line-height: 20px; } `;app/client/packages/design-system/ads/src/Templates/IDEHeader/index.ts (1)
14-14: Consider adding type exports for the HeaderSwitcher component.Since this is a design system component, consider exporting the prop types for better TypeScript integration.
export { IDEHeaderSwitcher } from "./HeaderSwitcher"; +export type { IDEHeaderSwitcherProps } from "./HeaderSwitcher";app/client/packages/design-system/ads/src/Templates/IDEHeader/IDEHeader.tsx (1)
5-7: Consider using a more specific type for childrenThe current type allows any ReactNode or array of ReactNodes. Consider using a more specific type if there are known constraints on the children components.
interface ChildrenProps { - children: React.ReactNode | React.ReactNode[]; + children: React.ReactNode; }app/client/packages/design-system/ads/src/Templates/IDEHeader/HeaderSwitcher/IDEHeaderSwitcher.tsx (5)
8-17: Add JSDoc documentation to Props interfaceConsider adding JSDoc documentation to describe the purpose of each prop, especially
titleTestIdwhich seems to be a required prop.+/** + * Props for the IDEHeaderSwitcher component + */ interface Props { + /** The prefix text displayed before the title */ prefix: string; + /** Optional title text */ title?: string; + /** Test ID for the title element - explain why this is required */ titleTestId: string; + /** Controls the expanded state of the switcher */ active: boolean; + /** Callback to update the active state */ setActive: (active: boolean) => void; + /** Optional click handler for the trigger element */ onClick?: React.MouseEventHandler<HTMLDivElement>; + /** Optional CSS class name */ className?: string; + /** Content to be rendered in the popover */ children: React.ReactNode; }
33-33: Consider using useMemo for separatorThe separator value could be memoized since it only depends on the title prop.
- const separator = title ? " /" : ""; + const separator = useMemo(() => title ? " /" : "", [title]);
44-44: Use template literal for classNameConsider using a template literal for better readability.
- className={`flex align-center items-center justify-center ${className}`} + className={`flex align-center items-center justify-center ${className ?? ''}`}
68-76: Simplify Icon color logicThe conditional color logic can be simplified using the nullish coalescing operator.
- color={ - title - ? undefined - : "var(--ads-v2-colors-content-label-inactive-fg)" - } + color={title ?? "var(--ads-v2-colors-content-label-inactive-fg)"}
39-84: Consider enhancing accessibilityThe component could benefit from additional ARIA attributes for better screen reader support.
Consider adding:
aria-expandedto indicate the expanded statearia-controlsto associate the trigger with the contentaria-labelfor better context<Styled.SwitchTrigger active={active} className={`flex align-center items-center justify-center ${className}`} data-testid={titleTestId} onClick={onClick} ref={ref} + aria-expanded={active} + aria-controls="switcher-content" + aria-label={`${prefix}${title ? ` ${title}` : ''} switcher`} {...rest} >app/client/packages/design-system/ads/src/Templates/IDEHeader/IDEHeader.stories.tsx (4)
12-12: Consider moving shared styles to a common locationImporting styles directly from another component (
EntityExplorer) breaks component isolation. Consider movingListHeaderContainerto a shared styles location if it's meant to be reused.
29-41: Add aria-labels and documentationThe Default story should:
- Include aria-labels for accessibility
- Add JSDoc comments describing the purpose and usage of each section
export const Default = () => ( <IDEHeader> - <IDEHeader.Left logo={<Icon name="upload-cloud" size="md" />}> + <IDEHeader.Left logo={<Icon name="upload-cloud" size="md" aria-label="Upload" />}> <span>Left Content</span> </IDEHeader.Left> - <IDEHeader.Center> + <IDEHeader.Center aria-label="Main content"> <span>Center Content</span> </IDEHeader.Center> - <IDEHeader.Right> + <IDEHeader.Right aria-label="Secondary actions"> <span>Right Content</span> </IDEHeader.Right> </IDEHeader> );
43-57: Remove empty divs and add test IDsThe story contains unnecessary empty divs and lacks test IDs for testing.
export const WithHeaderTitle = () => { return ( <IDEHeader> <IDEHeader.Left logo={<Icon name="upload-cloud" size="md" />}> - <IDEHeaderTitle title="Settings" /> + <IDEHeaderTitle title="Settings" data-testid="ide-header-title" /> </IDEHeader.Left> - <IDEHeader.Center> - <div /> - </IDEHeader.Center> - <IDEHeader.Right> - <div /> - </IDEHeader.Right> </IDEHeader> ); };
83-96: Extract list items as constantsMove the hardcoded list items to a constants file for better maintainability and reusability.
+const DEMO_PAGES = [ + { + title: "Page1", + onClick: noop, + description: "", + descriptionType: "inline", + }, + { + title: "Page2", + onClick: noop, + description: "", + descriptionType: "inline", + }, +]; <List - items={[ - { - title: "Page1", - onClick: noop, - description: "", - descriptionType: "inline", - }, - { - title: "Page2", - onClick: noop, - description: "", - descriptionType: "inline", - }, - ]} + items={DEMO_PAGES} />app/client/packages/design-system/ads/src/Templates/IDEHeader/HeaderSwitcher/HeaderSwitcher.test.tsx (3)
8-16: Consider adding TypeScript types for mock functions and default props.The test setup could benefit from explicit typing:
- const mockOnClick = jest.fn(); - const mockSetActive = jest.fn(); + const mockOnClick = jest.fn<void, []>(); + const mockSetActive = jest.fn<void, [boolean]>(); + type DefaultProps = React.ComponentProps<typeof IDEHeaderSwitcher>; + const defaultProps: DefaultProps = {
Line range hint
62-79: Refactor icon color test to use testing-library queries.The test uses container queries which are discouraged. Consider using data-testid for the icon.
- const { container } = render( + render( <IDEHeaderSwitcher {...defaultProps} data-testid="root-div" ref={ref} title={undefined} + iconTestId="chevron-icon" />, ); - const icon = container.querySelector(".remixicon-icon"); + const icon = screen.getByTestId("chevron-icon");
86-97: Use object spread for better test maintainability.The test props are duplicating defaultProps values unnecessarily.
- <IDEHeaderSwitcher - active - className={className} - data-testid={testId} - prefix="Prefix" - setActive={mockSetActive} - title="Title" - titleTestId="titleTestId" - > - <span>Test</span> - </IDEHeaderSwitcher> + <IDEHeaderSwitcher + {...defaultProps} + active + className={className} + data-testid={testId} + />
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (14)
app/client/packages/design-system/ads/src/Templates/EntityExplorer/index.ts(1 hunks)app/client/packages/design-system/ads/src/Templates/EntityExplorer/styles.ts(1 hunks)app/client/packages/design-system/ads/src/Templates/IDEHeader/HeaderSwitcher/HeaderSwitcher.styles.ts(1 hunks)app/client/packages/design-system/ads/src/Templates/IDEHeader/HeaderSwitcher/HeaderSwitcher.test.tsx(2 hunks)app/client/packages/design-system/ads/src/Templates/IDEHeader/HeaderSwitcher/IDEHeaderSwitcher.tsx(1 hunks)app/client/packages/design-system/ads/src/Templates/IDEHeader/HeaderSwitcher/index.ts(1 hunks)app/client/packages/design-system/ads/src/Templates/IDEHeader/IDEHeader.constants.ts(1 hunks)app/client/packages/design-system/ads/src/Templates/IDEHeader/IDEHeader.mdx(1 hunks)app/client/packages/design-system/ads/src/Templates/IDEHeader/IDEHeader.stories.tsx(1 hunks)app/client/packages/design-system/ads/src/Templates/IDEHeader/IDEHeader.tsx(1 hunks)app/client/packages/design-system/ads/src/Templates/IDEHeader/IDEHeaderTitle.tsx(1 hunks)app/client/packages/design-system/ads/src/Templates/IDEHeader/index.ts(1 hunks)app/client/packages/design-system/ads/src/Templates/index.ts(1 hunks)app/client/packages/design-system/ads/src/index.ts(1 hunks)
✅ Files skipped from review due to trivial changes (5)
- app/client/packages/design-system/ads/src/Templates/EntityExplorer/index.ts
- app/client/packages/design-system/ads/src/Templates/IDEHeader/HeaderSwitcher/index.ts
- app/client/packages/design-system/ads/src/Templates/IDEHeader/IDEHeader.constants.ts
- app/client/packages/design-system/ads/src/Templates/IDEHeader/IDEHeader.mdx
- app/client/packages/design-system/ads/src/Templates/index.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- app/client/packages/design-system/ads/src/index.ts
🔇 Additional comments (12)
app/client/packages/design-system/ads/src/Templates/IDEHeader/IDEHeaderTitle.tsx (1)
1-3: LGTM! Clean import structure
The imports are well-organized and use proper relative paths within the design system.
app/client/packages/design-system/ads/src/Templates/IDEHeader/HeaderSwitcher/HeaderSwitcher.styles.ts (1)
1-2: LGTM!
The imports are clean and follow the project structure.
app/client/packages/design-system/ads/src/Templates/EntityExplorer/styles.ts (1)
1-2: LGTM - Imports are clean and appropriate
app/client/packages/design-system/ads/src/Templates/IDEHeader/index.ts (2)
1-19: Well-structured barrel file with clear documentation.
The file is well-organized with clear JSDoc comments explaining the purpose of each component. The exports follow a logical grouping pattern.
7-8: Verify the constant exports usage across the codebase.
Let's ensure the constants are being used consistently across the application.
✅ Verification successful
Constants are used consistently across the codebase
The verification shows that both constants are being used appropriately:
IDE_HEADER_HEIGHTis consistently used for header height calculations and layout spacing across multiple componentsLOGO_WIDTHis properly scoped to the IDEHeader component where it's needed for logo sizing
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check usage of IDE_HEADER_HEIGHT and LOGO_WIDTH constants
# Search for IDE_HEADER_HEIGHT usage
echo "=== IDE_HEADER_HEIGHT usage ==="
rg "IDE_HEADER_HEIGHT" -A 2
# Search for LOGO_WIDTH usage
echo "=== LOGO_WIDTH usage ==="
rg "LOGO_WIDTH" -A 2
Length of output: 8578
app/client/packages/design-system/ads/src/Templates/IDEHeader/IDEHeader.tsx (5)
9-31: LGTM! Well-structured component with clear layout
The Left component implementation is clean with proper flex layout and prop typing.
33-45: LGTM! Clean and consistent implementation
The Center component follows the established pattern with proper flex layout.
47-60: LGTM! Consistent with other section components
The Right component maintains consistency with other sections while properly handling right alignment.
69-69: Use template literal for height value
As suggested in the previous review, using a template literal would be more robust.
-height={IDE_HEADER_HEIGHT + "px"}
+height={`${IDE_HEADER_HEIGHT}px`}78-80: LGTM! Appropriate component composition for ADS Templates
The namespacing approach is suitable for design system templates, making the component usage more intuitive and structured.
app/client/packages/design-system/ads/src/Templates/IDEHeader/HeaderSwitcher/IDEHeaderSwitcher.tsx (1)
88-88: LGTM!
Proper setting of displayName for the forwarded ref component.
app/client/packages/design-system/ads/src/Templates/IDEHeader/IDEHeader.stories.tsx (1)
75-75: Consider dynamic max-height based on viewport
Fixed maxHeight of 300px might clip content on smaller screens.
...nt/packages/design-system/ads/src/Templates/IDEHeader/HeaderSwitcher/HeaderSwitcher.test.tsx
Show resolved
Hide resolved
...nt/packages/design-system/ads/src/Templates/IDEHeader/HeaderSwitcher/HeaderSwitcher.test.tsx
Show resolved
Hide resolved
|
Deploy-Preview-URL: https://ce-37406.dp.appsmith.com |
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (4)
app/client/src/pages/Editor/IDE/Header/EditorTitle.tsx (1)
17-20: Consider improving type safetyThe type assertion
as stringsuggests we might need better typing from the selector. Consider defining proper return types forgetCurrentPageIdselector.// In editorSelectors.ts export const getCurrentPageId = (state: AppState): string | undefined => { // implementation };app/client/packages/design-system/ads/src/Templates/EntityExplorer/ListWithHeader.tsx (1)
6-11: Consider using a more flexible type for children.The current type
React.ReactNode[]requires children to be an array, which might be unnecessarily restrictive. Consider usingReact.ReactNodeinstead to allow both single and multiple children.interface Props { headerText: string; headerControls?: React.ReactNode; maxHeight?: string; - children: React.ReactNode[]; + children: React.ReactNode; }app/client/packages/design-system/ads/src/Templates/IDEHeader/IDEHeader.tsx (2)
5-7: Consider using a more specific type for childrenThe current type allows any ReactNode or array of ReactNodes. Consider using a union type of specific allowed components if there are restrictions on what can be rendered in these sections.
interface ChildrenProps { - children: React.ReactNode | React.ReactNode[]; + children: React.ReactElement | React.ReactElement[]; }
78-80: LGTM! Good use of the compound component patternThe static property pattern is a well-established React pattern (used by libraries like Material-UI) that provides good discoverability and enforces component relationships. This is a good architectural choice for template components.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (6)
app/client/packages/design-system/ads/src/Templates/EntityExplorer/ListWithHeader.tsx(1 hunks)app/client/packages/design-system/ads/src/Templates/EntityExplorer/index.ts(1 hunks)app/client/packages/design-system/ads/src/Templates/IDEHeader/IDEHeader.tsx(1 hunks)app/client/src/pages/Editor/IDE/EditorPane/PagesSection.tsx(2 hunks)app/client/src/pages/Editor/IDE/Header/EditorTitle.tsx(1 hunks)app/client/src/pages/Editor/IDE/Header/index.tsx(6 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- app/client/src/pages/Editor/IDE/EditorPane/PagesSection.tsx
- app/client/src/pages/Editor/IDE/Header/index.tsx
🔇 Additional comments (12)
app/client/packages/design-system/ads/src/Templates/EntityExplorer/index.ts (2)
1-1: LGTM! Clear and well-organized container exports.
The export of styled components follows the ADS pattern for providing reusable UI containers.
2-2: Verify the ListWithHeader implementation matches ADS Template guidelines.
The composite component should follow ADS Templates design principles of being a shallow component with clear slots for business logic integration.
✅ Verification successful
Based on the code inspection, I can now provide a final response:
ListWithHeader follows ADS Template guidelines
The component is a shallow template that:
- Takes clear props for header text, controls and children
- Uses composition with basic ADS components (Text, Flex)
- Contains no business logic or state management
- Provides clear slots for content integration via props
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify ListWithHeader follows ADS Template patterns
# Expected: Component should be shallow and provide clear slot props
# Check the implementation
ast-grep --pattern 'interface $_ {
$$$
children?: $_
$$$
}' ./app/client/packages/design-system/ads/src/Templates/EntityExplorer/ListWithHeader.tsx
# Check for business logic
rg -g '*.{ts,tsx}' 'useState|useEffect|useCallback' ./app/client/packages/design-system/ads/src/Templates/EntityExplorer/ListWithHeader.tsx
Length of output: 297
Script:
#!/bin/bash
# Let's first check if the file exists and see its content
cat ./app/client/packages/design-system/ads/src/Templates/EntityExplorer/ListWithHeader.tsx
# Backup: If .tsx doesn't exist, check .ts extension
if [ $? -ne 0 ]; then
cat ./app/client/packages/design-system/ads/src/Templates/EntityExplorer/ListWithHeader.ts
fi
# Let's also check for any props interface or type definitions
ast-grep --pattern 'type $_ = {
$$$
}'
# And check for the component definition
ast-grep --pattern 'const $_ = ($_: $_) => {
$$$
}'
Length of output: 100342
app/client/src/pages/Editor/IDE/Header/EditorTitle.tsx (4)
1-8: LGTM! Clean import organization
The imports are well-organized and align with the migration to ADS. Good adoption of the useBoolean hook as previously suggested.
10-15: Well-structured state management with useBoolean
Clean implementation with descriptive naming for the boolean state handlers. The component's interface has been simplified by removing props.
23-31: Clean implementation of IDEHeaderSwitcher
The component properly implements the new ADS template pattern with appropriate props and test IDs. The integration with PagesSection maintains good separation of concerns.
2-2: Verify complete migration to ADS
Let's ensure all IDE header components have been moved to ADS consistently.
✅ Verification successful
Let me check the actual content of these files to verify the migration status.
IDE header components properly migrated to ADS
The verification shows that all IDE header components are correctly using the ADS imports:
- Main header components (
IDEHeader,IDEHeaderTitle,IDEHeaderSwitcher) are imported from@appsmith/ads - All usages in
Header/index.tsxandEditorTitle.tsxare using the ADS components - Other references are just imports or route definitions, not component implementations
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for any remaining IDE header components outside ADS
rg -g '!node_modules' -g '!dist' -g '!build' -l "IDE.*Header" ./app/client/src/
Length of output: 441
Script:
#!/bin/bash
# Check content of IDE header related files
rg -g '!node_modules' -g '!dist' -g '!build' "IDE.*Header" ./app/client/src/ -A 2
Length of output: 5743
app/client/packages/design-system/ads/src/Templates/EntityExplorer/ListWithHeader.tsx (2)
1-4: LGTM! Clean and well-organized imports.
21-21: Review the usage of className="pages".
The className "pages" seems arbitrary and could potentially conflict with other styles. Consider using a more specific and scoped class name that reflects the component's purpose.
app/client/packages/design-system/ads/src/Templates/IDEHeader/IDEHeader.tsx (4)
9-31: LGTM! Well-structured layout component
The Left component is well implemented with proper use of design system variables and clear class names for testing.
33-45: LGTM! Consistent with design patterns
The Center component maintains consistency with other sections and properly implements centering behavior.
47-60: LGTM! Maintains consistency
The Right component follows the established pattern and properly handles right-aligned content.
62-76: LGTM! Clean implementation of the main container
The main IDEHeader component is well implemented. The direct usage of pixel height is preferred here as it's more type-safe than using template literals.
app/client/packages/design-system/ads/src/Templates/EntityExplorer/ListWithHeader.tsx
Outdated
Show resolved
Hide resolved
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/Templates/EntityExplorer/ListWithHeader.tsx (1)
6-12: Add JSDoc comments and consider narrowing children type.The Props interface would benefit from documentation and type improvements.
+/** + * Props for the ListWithHeader component + * @property headerText - Text to display in the header + * @property headerControls - Optional controls to render in the header + * @property maxHeight - Optional maximum height for the container + * @property headerClassName - Optional class name for header styling + * @property children - Content to render in the list body + */ interface Props { headerText: string; headerControls?: React.ReactNode; maxHeight?: string; headerClassName?: string; - children: React.ReactNode | React.ReactNode[]; + children: React.ReactNode; // ReactNode already includes arrays }
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (3)
app/client/packages/design-system/ads/src/Templates/EntityExplorer/ListWithHeader.tsx(1 hunks)app/client/packages/design-system/ads/src/Templates/EntityExplorer/styles.ts(1 hunks)app/client/src/pages/Editor/IDE/EditorPane/PagesSection.tsx(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- app/client/packages/design-system/ads/src/Templates/EntityExplorer/styles.ts
- app/client/src/pages/Editor/IDE/EditorPane/PagesSection.tsx
🔇 Additional comments (2)
app/client/packages/design-system/ads/src/Templates/EntityExplorer/ListWithHeader.tsx (2)
1-4: LGTM! Clean imports.
14-38: Add data-testid props for testing.
Consider adding test ID props to facilitate testing of this component.
export const ListWithHeader = (props: Props) => {
return (
<Flex
+ data-testid="list-with-header"
flexDirection="column"
justifyContent="center"
maxHeight={props.maxHeight}
overflow="hidden"
>
- <ListHeaderContainer className={props.headerClassName}>
+ <ListHeaderContainer
+ className={props.headerClassName}
+ data-testid="list-header"
+ >
<Text kind="heading-xs">{props.headerText}</Text>
{props.headerControls}
</ListHeaderContainer>
<Flex
+ data-testid="list-content"
alignItems="center"
flex="1"
flexDirection="column"
overflow="auto"
px="spaces-2"
width="100%"
>
{props.children}
</Flex>
</Flex>
);
};The previous review comment about enhancing accessibility with semantic markup is still valid and should be addressed.
app/client/packages/design-system/ads/src/Templates/IDEHeader/HeaderSwitcher/index.ts
Show resolved
Hide resolved
|
/build-deploy-preview skip-tests=true |
|
Deploying Your Preview: https://github.com/appsmithorg/appsmith/actions/runs/12007276646. |
|
Deploy-Preview-URL: https://ce-37406.dp.appsmith.com |
This reverts commit 8cb0bee.
This reverts commit 8cb0bee. <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit - **New Features** - Introduced `HeaderEditorSwitcher` and `HeaderDropdown` components for enhanced UI interactions. - **Bug Fixes** - Updated import paths for `IDE_HEADER_HEIGHT` across multiple components to ensure consistent styling. - **Refactor** - Renamed and restructured components for clarity and improved functionality, including `HeaderTitle` and `IDEHeaderSwitcher`. - **Tests** - Added tests for the `HeaderDropdown` component to validate rendering and functionality. - **Chores** - Removed deprecated components and files to streamline the codebase. <!-- end of auto-generated comment: release notes by coderabbit.ai -->
## Description Extracting out our IDE Header component into ADS for better usability. ADS Templates are shallow components built using opinionated ADS which provide "slots" to place other business logic components inside it. It reduces the work of using ADS by providing pre built UI components Also creating the EntityExplorer folder and created ListWithHeader as a component. Will keep updating this ADS template as we work on Entity Explorer modularization Fixes appsmithorg#37607 ## Automation /ok-to-test tags="@tag.All" ### 🔍 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/11971098012> > Commit: e28c56e > <a href="https://internal.appsmith.com/app/cypress-dashboard/rundetails-65890b3c81d7400d08fa9ee5?branch=master&workflowId=11971098012&attempt=2" target="_blank">Cypress dashboard</a>. > Tags: `@tag.All` > Spec: > <hr>Fri, 22 Nov 2024 13:13:39 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 ## Release Notes - **New Features** - Introduced the `IDEHeader` component with subcomponents (`Left`, `Center`, `Right`) for improved header layout. - Added `IDEHeaderSwitcher` for enhanced navigation within the header. - New styled components `ListItemContainer`, `ListHeaderContainer`, and `ListWithHeader` for the entity explorer interface. - Added new constants `IDE_HEADER_HEIGHT` and `LOGO_WIDTH` for standardized dimensions in the header layout. - **Improvements** - Updated header titles and constants for better clarity in the user interface. - Enhanced layout and styling of the `PagesSection` component for improved user experience. - Consolidated import statements for better organization across various components. - Removed deprecated components and tests to streamline the codebase. - **Documentation** - Added documentation for the `IDEHeader` component, including stories for visual representation. <!-- end of auto-generated comment: release notes by coderabbit.ai -->
…ppsmithorg#37739) This reverts commit 8cb0bee. <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit - **New Features** - Introduced `HeaderEditorSwitcher` and `HeaderDropdown` components for enhanced UI interactions. - **Bug Fixes** - Updated import paths for `IDE_HEADER_HEIGHT` across multiple components to ensure consistent styling. - **Refactor** - Renamed and restructured components for clarity and improved functionality, including `HeaderTitle` and `IDEHeaderSwitcher`. - **Tests** - Added tests for the `HeaderDropdown` component to validate rendering and functionality. - **Chores** - Removed deprecated components and files to streamline the codebase. <!-- end of auto-generated comment: release notes by coderabbit.ai -->
Description
Extracting out our IDE Header component into ADS for better usability.
ADS Templates are shallow components built using opinionated ADS which provide "slots" to place other business logic components inside it. It reduces the work of using ADS by providing pre built UI components
Also creating the EntityExplorer folder and created ListWithHeader as a component. Will keep updating this ADS template as we work on Entity Explorer modularization
Fixes #37607
Automation
/ok-to-test tags="@tag.All"
🔍 Cypress test results
Tip
🟢 🟢 🟢 All cypress tests have passed! 🎉 🎉 🎉
Workflow run: https://github.com/appsmithorg/appsmith/actions/runs/11971098012
Commit: e28c56e
Cypress dashboard.
Tags:
@tag.AllSpec:
Fri, 22 Nov 2024 13:13:39 UTC
Communication
Should the DevRel and Marketing teams inform users about this change?
Summary by CodeRabbit
Release Notes
New Features
IDEHeadercomponent with subcomponents (Left,Center,Right) for improved header layout.IDEHeaderSwitcherfor enhanced navigation within the header.ListItemContainer,ListHeaderContainer, andListWithHeaderfor the entity explorer interface.IDE_HEADER_HEIGHTandLOGO_WIDTHfor standardized dimensions in the header layout.Improvements
PagesSectioncomponent for improved user experience.Documentation
IDEHeadercomponent, including stories for visual representation.