chore: Move IDE header to ADS/Templates #37764
Conversation
WalkthroughA new React component, 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
IDE Navigation
hetunandu
added
ok-to-test
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: 3
🧹 Outside diff range and nitpick comments (24)
app/client/packages/design-system/ads/src/Templates/IDEHeader/IDEHeaderTitle.tsx (1)
5-7: Enhance documentation with JSDoc format.Consider adding proper JSDoc with @param documentation.
-/** - * Handy little styled component that can be used to render the title in the IDEHeader component - **/ +/** + * A styled component that renders the title in the IDEHeader component + * @param {Object} props - Component props + * @param {string} props.title - The title text to display + */app/client/packages/design-system/ads/src/Templates/IDEHeader/HeaderSwitcher/HeaderSwitcher.styles.ts (2)
4-15: Consider enhancing visual states and simplifying the style implementation.The hover and active states currently use the same background color, which might not provide enough visual distinction for users.
Consider this implementation:
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"}; + background-color: ${({ active }) => active && 'var(--ads-v2-color-bg-subtle)'}; cursor: pointer; padding: var(--ads-v2-spaces-2); :hover { - background-color: var(--ads-v2-color-bg-subtle); + background-color: var(--ads-v2-color-bg-muted); } `;
17-20: Use design system spacing variables for consistency.Consider using the design system spacing variables instead of em units to maintain consistency.
export const ContentContainer = styled(PopoverContent)` padding: 0; - padding-bottom: 0.25em; + padding-bottom: var(--ads-v2-spaces-2); `;app/client/packages/design-system/ads/src/Templates/IDEHeader/index.ts (2)
16-19: Consider enhancing documentation with styling details.While the purpose is clear, it would be helpful to document any specific styling constraints or customization options available for this wrapper component.
1-19: Well-structured modular design that aligns with ADS principles.The composable header pattern with clear separation of concerns provides a solid foundation for the Application Development System. The modular approach will facilitate future extensions and customizations of the IDE header.
app/client/packages/design-system/ads/src/Templates/EntityExplorer/ListWithHeader.tsx (3)
6-12: Add JSDoc documentation for the Props interface.Adding documentation will improve component usability and maintainability.
+/** + * Props for the ListWithHeader component + */ interface Props { + /** Text to be displayed in the header */ headerText: string; + /** Optional controls to be displayed in the header */ headerControls?: React.ReactNode; + /** Optional maximum height for the container */ maxHeight?: string; + /** Optional className for the header container */ headerClassName?: string; + /** Content to be displayed in the list body */ children: React.ReactNode | React.ReactNode[]; }
22-25: Enhance accessibility with ARIA attributes.Add appropriate ARIA attributes for better screen reader support.
- <ListHeaderContainer className={props.headerClassName}> + <ListHeaderContainer + className={props.headerClassName} + role="heading" + aria-level={1} + >
26-35: Consider making padding configurable.The fixed padding (
px="spaces-2") might not suit all use cases. Consider making it configurable through props.interface Props { headerText: string; headerControls?: React.ReactNode; maxHeight?: string; headerClassName?: string; + contentPadding?: string; children: React.ReactNode | React.ReactNode[]; }<Flex alignItems="center" flex="1" flexDirection="column" overflow="auto" - px="spaces-2" + px={props.contentPadding ?? "spaces-2"} width="100%" >app/client/packages/design-system/ads/src/index.ts (1)
39-39: Consider maintaining alphabetical order of exportsThe new Templates export should be placed between Table and Text exports to maintain consistency with the existing alphabetical ordering.
export * from "./Switch"; export * from "./Tab"; export * from "./Table"; +export * from "./Templates"; export * from "./Text"; export * from "./Toast"; -export * from "./Templates";app/client/src/pages/Editor/IDE/Header/EditorTitle.tsx (1)
17-20: Consider adding error handling for missing pageWhile the null check prevents crashes, consider adding error feedback for better UX when a page isn't found.
const pageId = useSelector(getCurrentPageId) as string; const currentPage = useSelector(getPageById(pageId)); - if (!currentPage) return null; + if (!currentPage) { + return <div>Page not found</div>; // Or use your design system's error component + }app/client/packages/design-system/ads/src/Templates/IDEHeader/IDEHeader.tsx (4)
9-31: Consider strengthening type safety and testability.
- The
logoprop type could be more specific thanReact.ReactNode- Add data-testid attributes for testing purposes
-const Left = (props: PropsWithChildren<{ logo: React.ReactNode }>) => { +type LogoProps = { logo: JSX.Element }; +const Left = (props: PropsWithChildren<LogoProps>) => { return ( <Flex alignItems="center" className="header-left-section" + data-testid="ide-header-left" flex="1"
33-45: Add test identifier for consistency.Add data-testid attribute to match the testing pattern used in other sections.
return ( <Flex alignItems="center" className="header-center-section" + data-testid="ide-header-center" flex="1"
47-60: Add test identifier for consistency.Add data-testid attribute to maintain testing pattern consistency.
return ( <Flex alignItems="center" className="header-right-section" + data-testid="ide-header-right" flex="1"
62-76: Consider performance optimizations.The component might benefit from memoization if it receives frequent prop updates from parent components.
-export const IDEHeader = (props: ChildrenProps) => { +export const IDEHeader = React.memo((props: ChildrenProps) => { return ( <Flex alignItems="center"app/client/packages/design-system/ads/src/Templates/IDEHeader/HeaderSwitcher/IDEHeaderSwitcher.tsx (4)
8-17: Consider making titleTestId optional if title is optionalThe interface requires
titleTestIdbut makestitleoptional. This could lead to scenarios where we have a test ID for a non-existent title.interface Props { prefix: string; title?: string; - titleTestId: string; + titleTestId?: string; active: boolean; setActive: (active: boolean) => void; onClick?: React.MouseEventHandler<HTMLDivElement>; className?: string; children: React.ReactNode; }
33-37: Consider making the separator logic more explicitThe separator logic could be more readable with a ternary operator or explicit condition.
- const separator = title ? " /" : ""; + const separator = Boolean(title) ? " /" : "";
44-44: Use template literals for className concatenationConsider using template literals for better readability when combining classNames.
- className={`flex align-center items-center justify-center ${className}`} + className={`flex align-center items-center justify-center ${className ?? ''}`}
59-59: Consider typing the data-active attributeThe data-active attribute should be properly typed for better TypeScript integration.
+ type DataAttributes = { + 'data-active'?: boolean; + }; + + interface Props extends DataAttributes { // ... existing props + }app/client/packages/design-system/ads/src/Templates/IDEHeader/IDEHeader.stories.tsx (4)
5-5: Fix inconsistent import path for IDEHeaderSwitcherThe import path "./HeaderSwitcher" doesn't match the component naming convention. Consider renaming the file to "IDEHeaderSwitcher.tsx" for consistency.
49-54: Consider removing empty divsEmpty divs in Center and Right sections can be replaced with null or removed entirely if they serve no purpose.
- <IDEHeader.Center> - <div /> - </IDEHeader.Center> - <IDEHeader.Right> - <div /> - </IDEHeader.Right> + <IDEHeader.Center /> + <IDEHeader.Right />
75-75: Extract magic number to a constantThe maxHeight value "300px" should be defined as a constant for better maintainability.
+const MAX_DROPDOWN_HEIGHT = "300px"; + export const WithHeaderDropdown = () => { // ... - maxHeight={"300px"} + maxHeight={MAX_DROPDOWN_HEIGHT}
83-96: Extract list items to a constantConsider extracting the list items array to a constant 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/src/pages/Editor/IDE/EditorPane/PagesSection.tsx (2)
59-63: Consider memoizing the onClick callbackWhile the current implementation is correct, consider memoizing the
onClickcallback passed to PageElement to prevent unnecessary re-renders.+ const handleItemClick = useCallback(() => onItemSelected(), [onItemSelected]); const pageElements = useMemo( () => pages.map((page) => ( <ListItemContainer key={page.pageId}> - <PageElement onClick={onItemSelected} page={page} /> + <PageElement onClick={handleItemClick} page={page} /> </ListItemContainer> )), - [pages, location.pathname, onItemSelected], + [pages, location.pathname, handleItemClick], );
88-95: Consider making maxHeight configurableThe hardcoded maxHeight value might limit component reusability. Consider making it configurable through component props.
+ interface PagesSectionProps { + onItemSelected: () => void; + maxHeight?: string; + } - const PagesSection = ({ onItemSelected }: { onItemSelected: () => void }) => { + const PagesSection = ({ maxHeight = "300px", onItemSelected }: PagesSectionProps) => { // ... return ( <ListWithHeader headerClassName={"pages"} headerControls={createPageContextMenu} headerText={`All Pages (${pages.length})`} - maxHeight={"300px"} + maxHeight={maxHeight} >
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (32)
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/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/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)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.test.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(2 hunks)app/client/src/pages/Editor/IDE/Header/EditorTitle.tsx(1 hunks)app/client/src/pages/Editor/IDE/Header/index.tsx(6 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 (5)
- app/client/src/IDE/Components/HeaderDropdown.test.tsx
- app/client/src/IDE/Components/HeaderDropdown.tsx
- app/client/src/IDE/Components/HeaderEditorSwitcher.test.tsx
- app/client/src/IDE/Components/HeaderEditorSwitcher.tsx
- app/client/src/IDE/index.ts
✅ Files skipped from review due to trivial changes (13)
- app/client/packages/design-system/ads/src/Templates/EntityExplorer/index.ts
- app/client/packages/design-system/ads/src/Templates/EntityExplorer/styles.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
- 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/EditorHeaderComponents.tsx
- app/client/src/pages/Editor/commons/Omnibar.tsx
- app/client/src/pages/Editor/index.tsx
🔇 Additional comments (22)
app/client/packages/design-system/ads/src/Templates/IDEHeader/IDEHeaderTitle.tsx (1)
Line range hint 8-17: LGTM! Verify component usage after renaming.
The implementation is clean and the naming change aligns with the move to ADS.
✅ Verification successful
Component rename is properly implemented across the codebase
All references to the new IDEHeaderTitle component are correctly used in the codebase. The component is:
- Properly exported from the design system
- Correctly imported and used in stories
- Successfully integrated in the IDE Header implementation
- No stray references to the old name exist that would need updating
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify that all usages of HeaderTitle have been updated to IDEHeaderTitle
# Search for any remaining references to HeaderTitle
rg "HeaderTitle" --type ts --type tsx
# Search for usage of the new component name
rg "IDEHeaderTitle" --type ts --type tsx
Length of output: 134
Script:
#!/bin/bash
# Search for any remaining references to HeaderTitle without file type restrictions
rg "HeaderTitle"
# Search for usage of the new component name without file type restrictions
rg "IDEHeaderTitle"
Length of output: 3507
app/client/packages/design-system/ads/src/Templates/IDEHeader/HeaderSwitcher/HeaderSwitcher.styles.ts (1)
1-2: LGTM! Clean imports.
app/client/packages/design-system/ads/src/Templates/IDEHeader/index.ts (2)
10-14: LGTM! Clear documentation and purpose.
The component's purpose is well-documented with a practical example of its usage.
1-8: LGTM! Verify constants usage across the application.
The modular export structure and documentation are well-organized. The separation of constants into a dedicated file follows best practices.
Let's verify the usage of these constants across the application:
✅ Verification successful
Constants are properly used across the application
The IDE_HEADER_HEIGHT and LOGO_WIDTH constants are consistently used throughout the application for:
- Layout calculations in editor components
- Header sizing in multiple views
- Navigation and sidebar positioning
- Consistent spacing in admin settings
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check usage of IDE_HEADER_HEIGHT and LOGO_WIDTH constants
rg -A 2 "IDE_HEADER_HEIGHT|LOGO_WIDTH" --type ts --type tsx
Length of output: 89
Script:
#!/bin/bash
# Search for IDE_HEADER_HEIGHT and LOGO_WIDTH constants using ripgrep with correct file type
rg -A 2 "IDE_HEADER_HEIGHT|LOGO_WIDTH" --type typescript --type-add 'typescript:*.{ts,tsx}'
Length of output: 7755
app/client/packages/design-system/ads/src/Templates/EntityExplorer/ListWithHeader.tsx (1)
1-4: LGTM! Clean imports following design system patterns.
app/client/packages/design-system/ads/src/index.ts (1)
39-39: Verify Templates module structure
The export looks good, but let's verify the Templates module structure aligns with the PR objectives for IDE Header components.
✅ Verification successful
Templates module structure is well-organized and properly exports IDE Header components
The Templates module has a clean structure with proper exports of the IDE Header components. The module includes:
- A composable IDEHeader component with Left/Center/Right sections
- Well-documented exports including IDEHeaderSwitcher and IDEHeaderTitle
- Properly defined constants for header dimensions
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify the Templates module structure and IDE Header components
# Check Templates directory structure
fd --type d "Templates" --exec tree {}
# Verify IDE Header components exist in Templates
rg -l "IDEHeader" --type ts --type tsx
Length of output: 3210
Script:
#!/bin/bash
# Let's properly check for IDE Header components with correct file type options
rg -l "IDEHeader" -t typescript -t tsx
# Check the content of IDEHeader module
cat "./app/client/packages/design-system/ads/src/Templates/IDEHeader/IDEHeader.tsx"
Length of output: 1860
Script:
#!/bin/bash
# Let's check the exports and structure of the Templates directory
cat "./app/client/packages/design-system/ads/src/Templates/index.ts"
# Check if IDEHeader is properly exported and structured
cat "./app/client/packages/design-system/ads/src/Templates/IDEHeader/index.ts"
# Let's also verify the constants being used
cat "./app/client/packages/design-system/ads/src/Templates/IDEHeader/IDEHeader.constants.ts"
Length of output: 1135
app/client/src/pages/Editor/IDE/Header/EditorTitle.tsx (3)
1-8: LGTM! Clean import organization
The imports reflect the component's transition to ADS and improved state management practices.
10-15: LGTM! Clean hook implementation
Good use of useBoolean hook with descriptive method names for menu state management.
23-31: LGTM! Clean implementation of IDEHeaderSwitcher
The implementation aligns well with the ADS migration objectives. Props are well-structured and include test IDs.
Let's verify the component's usage pattern:
✅ Verification successful
Implementation verified - usage is consistent with design system
The component is properly imported from the design system and used according to its documented purpose for page switching in the header. Test coverage is in place with the t--pages-switcher selector being referenced in Cypress tests.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check for consistent usage of IDEHeaderSwitcher across the codebase
rg -l "IDEHeaderSwitcher"
# Verify test coverage
rg -l "t--pages-switcher"
Length of output: 546
Script:
#!/bin/bash
# Check the implementation of IDEHeaderSwitcher to verify props and usage pattern
ast-grep --pattern 'interface $_Props {
$$$
}' app/client/packages/design-system/ads/src/Templates/IDEHeader/HeaderSwitcher/IDEHeaderSwitcher.tsx
# Check other usages of IDEHeaderSwitcher to verify consistency
rg "IDEHeaderSwitcher" -A 5 -B 2
Length of output: 7419
app/client/src/pages/Editor/commons/EditorWrapperContainer.tsx (1)
7-7: LGTM! Import path change aligns with ADS migration.
The change to import IDE_HEADER_HEIGHT from @appsmith/ads is consistent with the PR objective of moving IDE components to ADS/Templates.
app/client/packages/design-system/ads/src/Templates/IDEHeader/IDEHeader.tsx (2)
1-7: LGTM! Clean imports and type definitions.
78-80: LGTM! Clean compound component implementation.
app/client/src/pages/Editor/JSEditor/styledComponents.ts (1)
9-9: LGTM! Verify consistent imports across the codebase.
The import change aligns with the PR objective of moving IDE components to ADS.
Let's verify that all imports of IDE_HEADER_HEIGHT are consistently sourced from @appsmith/ads:
✅ Verification successful
All imports of IDE_HEADER_HEIGHT are consistently using @appsmith/ads
The verification shows that all imports of IDE_HEADER_HEIGHT across the codebase are using the new path "@appsmith/ads". No instances of the old import from "IDE" were found.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for any remaining imports of IDE_HEADER_HEIGHT from the old "IDE" path
# and verify consistent usage of the new path
# Check for any remaining old imports
rg "import.*IDE_HEADER_HEIGHT.*from.*\"IDE\""
# Verify new imports
rg "import.*IDE_HEADER_HEIGHT.*from.*\"@appsmith/ads\""
Length of output: 1072
app/client/packages/design-system/ads/src/Templates/IDEHeader/HeaderSwitcher/IDEHeaderSwitcher.tsx (2)
19-31: LGTM! Proper use of forwardRef and props destructuring
The component follows React best practices with proper ref forwarding and clean props handling.
88-88: LGTM! Proper display name setup
The display name is correctly set for better debugging experience.
app/client/packages/design-system/ads/src/Templates/IDEHeader/IDEHeader.stories.tsx (1)
29-41: LGTM! Well-structured default story
The story effectively demonstrates the basic layout and usage of the IDEHeader component.
app/client/src/pages/Editor/IDE/EditorPane/PagesSection.tsx (3)
1-2: LGTM! Import changes align with ADS migration
The replacement of components with ADS equivalents follows the PR's objective of moving IDE components to the design system.
52-52: LGTM! Proper dependency array update
The dependency array correctly includes all values used in the callback.
66-85: LGTM! Well-structured context menu implementation
The memoized implementation with proper permission checks and complete dependency array follows React best practices.
app/client/src/pages/Editor/IDE/Header/index.tsx (3)
16-17: LGTM: Import changes align with ADS migration
The new imports from @appsmith/ads align with the PR objective of moving IDE header components to ADS/Templates.
Also applies to: 81-81
221-224: LGTM: Clean header structure with proper component composition
The implementation follows a clean structure using IDEHeader's Left/Center/Right components and properly integrates the AppsmithLink logo.
Line range hint 95-118: Verify EditorTitle behavior with empty props
The component now relies on EditorTitle without passing any props. Please verify that EditorTitle handles this case gracefully.
✅ Verification successful
EditorTitle handles empty props case appropriately
The component is designed to work without props, using Redux selectors to get required data. It has proper null handling when page data is unavailable.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check EditorTitle implementation for default prop handling
ast-grep --pattern 'const EditorTitle = ({ $_ }) {
$$$
}'
Length of output: 61
Script:
#!/bin/bash
# Let's try a broader search for EditorTitle component
rg "EditorTitle" -A 10
# Also search for any exports of EditorTitle
rg "export.*EditorTitle"
# Search for EditorTitle using a more flexible AST pattern
ast-grep --pattern 'EditorTitle = $_'
Length of output: 145930
Script:
#!/bin/bash
# Let's try to find the EditorTitle component implementation with more context
rg -A 15 "const EditorTitle = \(\)" app/client/src/pages/Editor/IDE/Header/EditorTitle.tsx
Length of output: 494
app/client/packages/design-system/ads/src/Templates/EntityExplorer/ListWithHeader.tsx
Show resolved
Hide resolved
app/client/packages/design-system/ads/src/Templates/IDEHeader/IDEHeader.stories.tsx
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Outside diff range and nitpick comments (18)
app/client/packages/design-system/ads/src/Templates/IDEHeader/IDEHeaderTitle.tsx (2)
5-7: Enhance component documentation with JSDoc tagsConsider adding proper JSDoc tags to document the component props and return type.
/** + * @component * Handy little styled component that can be used to render the title in the IDEHeader component + * @param {Object} props + * @param {string} props.title - The title text to be displayed + * @returns {JSX.Element} A styled title component */
Line range hint
8-16: Consider extracting props interface for better maintainabilityThe implementation looks good and follows React best practices. For better maintainability and reusability, consider extracting the props interface.
+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 enhancing the component's interactivity and accessibility.The component could benefit from:
- Smooth state transitions
- Distinct hover state
- Focus state for keyboard navigation
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); + background-color: var(--ads-v2-color-bg-muted); } + + :focus-visible { + outline: 2px solid var(--ads-v2-color-border-focus); + outline-offset: -2px; + } `;
17-20: Maintain consistency with design system spacing variables.Replace the hardcoded
0.25emwith the appropriate 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/Templates/IDEHeader/index.ts (2)
1-8: LGTM! Consider enhancing type safety with explicit exports.The documentation and organization are clear. The composable pattern with Left/Center/Right sections follows React best practices.
Consider being explicit about the types:
-export { IDEHeader } from "./IDEHeader"; +export type { IDEHeaderProps } from "./IDEHeader"; +export { IDEHeader } from "./IDEHeader";
10-14: Enhance JSDoc with props documentation.While the purpose is clear, adding props documentation would improve developer experience.
Consider expanding the JSDoc:
/** * IDEHeaderSwitcher can be used for a trigger component to show a dropdown for pages, modules * or any list of elements in the header E.g., Pages / Page 1 + * + * @param {Object} props + * @param {Array<T>} props.items - List of items to display in the dropdown + * @param {(item: T) => void} props.onSelect - Callback when an item is selected */app/client/packages/design-system/ads/src/Templates/EntityExplorer/ListWithHeader.tsx (1)
6-12: Consider enhancing prop types and documentation.
- The
maxHeightprop could be more strictly typed using a union type of common CSS values or a custom type.- Adding JSDoc comments would improve component documentation.
interface Props { + /** The text to display in the header */ headerText: string; + /** Optional controls to display in the header */ headerControls?: React.ReactNode; + /** Maximum height of the container. Example: "100px", "50vh" */ - maxHeight?: string; + maxHeight?: `${number}${'px' | 'vh' | '%'}`; + /** Optional className for the header container */ headerClassName?: string; + /** Content to render in the scrollable area */ children: React.ReactNode | React.ReactNode[]; }app/client/packages/design-system/ads/src/Templates/IDEHeader/IDEHeader.tsx (2)
5-7: Consider using a more specific type for children arrayThe children type could be more specific about the array type.
- children: React.ReactNode | React.ReactNode[]; + children: React.ReactNode | readonly React.ReactNode[];
47-60: Consider maintaining consistent gap spacingThe gap spacing differs between Left (spaces-4) and Right (spaces-3) sections.
- gap="spaces-3" + gap="spaces-4"app/client/packages/design-system/ads/src/Templates/IDEHeader/HeaderSwitcher/IDEHeaderSwitcher.tsx (4)
8-17: Consider enhancing accessibility and prop consistencyThe Props interface could benefit from additional accessibility properties and consistent optional/required patterns.
Consider these improvements:
interface Props { prefix: string; title?: string; - titleTestId: string; + titleTestId?: string; + ariaLabel?: string; + ariaExpanded?: boolean; active: boolean; setActive: (active: boolean) => void; onClick?: React.MouseEventHandler<HTMLDivElement>; className?: string; children: React.ReactNode; }
33-34: Consider extracting separator logicThe separator logic could be moved to a constant or utility function for better maintainability.
+const SEPARATOR = " /"; + const separator = title ? " /" : "";
42-49: Improve className handlingConsider using a className utility library like
clsxfor more robust className merging.+import clsx from 'clsx'; + <Styled.SwitchTrigger active={active} - className={`flex align-center items-center justify-center ${className}`} + className={clsx('flex align-center items-center justify-center', className)} data-testid={titleTestId} onClick={onClick} ref={ref} {...rest} >
69-73: Extract color constantsConsider moving the hardcoded color values to a theme constants file.
+const COLORS = { + labelInactive: 'var(--ads-v2-colors-content-label-inactive-fg)', +} as const; + color={ title ? undefined - : "var(--ads-v2-colors-content-label-inactive-fg)" + : COLORS.labelInactive }app/client/packages/design-system/ads/src/Templates/IDEHeader/IDEHeader.stories.tsx (5)
12-12: Consider moving ListHeaderContainer to a shared locationImporting styles from EntityExplorer creates a direct dependency between templates. Consider moving the shared styles to a common location within the design system.
29-41: Consider adding more realistic demo contentWhile the structure is clear, using more realistic content would better demonstrate the component's intended usage.
- <span>Left Content</span> + <Text kind="heading-xs">Dashboard</Text>
49-54: Remove unnecessary empty divsIf Center and Right sections are not needed for this story, consider omitting them entirely rather than using empty divs.
- <IDEHeader.Center> - <div /> - </IDEHeader.Center> - <IDEHeader.Right> - <div /> - </IDEHeader.Right>
75-75: Consider making maxHeight configurableA fixed maxHeight of 300px might not work for all use cases. Consider making it a prop or using CSS variables for better flexibility.
- maxHeight={"300px"} + maxHeight={props.maxHeight ?? "300px"}
83-96: Extract list items to constantsConsider moving the list items to a separate constant or mock data file for better maintainability.
+const DEMO_PAGES = [ + { + title: "Page1", + onClick: () => console.log("Page1 clicked"), + description: "", + descriptionType: "inline", + }, + { + title: "Page2", + onClick: () => console.log("Page2 clicked"), + description: "", + descriptionType: "inline", + }, +]; - items={[ - { - title: "Page1", - onClick: noop, - description: "", - descriptionType: "inline", - }, - { - title: "Page2", - onClick: noop, - description: "", - descriptionType: "inline", - }, - ]} + items={DEMO_PAGES}
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (32)
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/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/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)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.test.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(2 hunks)app/client/src/pages/Editor/IDE/Header/EditorTitle.tsx(1 hunks)app/client/src/pages/Editor/IDE/Header/index.tsx(6 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 (5)
- app/client/src/IDE/Components/HeaderDropdown.test.tsx
- app/client/src/IDE/Components/HeaderDropdown.tsx
- app/client/src/IDE/Components/HeaderEditorSwitcher.test.tsx
- app/client/src/IDE/Components/HeaderEditorSwitcher.tsx
- app/client/src/IDE/index.ts
✅ Files skipped from review due to trivial changes (14)
- app/client/packages/design-system/ads/src/Templates/EntityExplorer/index.ts
- app/client/packages/design-system/ads/src/Templates/EntityExplorer/styles.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
- app/client/src/ce/constants/messages.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/EditorHeaderComponents.tsx
- app/client/src/pages/Editor/commons/Omnibar.tsx
- app/client/src/pages/Editor/index.tsx
🔇 Additional comments (28)
app/client/src/pages/Editor/IDE/Header/EditorTitle.tsx (4)
1-8: LGTM
The imports are correctly managed, and dependencies are properly organized.
10-15: LGTM
State management with useBoolean simplifies the component's logic.
17-20: LGTM
Retrieving page data from the Redux store and handling the null case ensures robustness.
23-31: LGTM
The IDEHeaderSwitcher is implemented correctly with appropriate props, and the integration of PagesSection aligns with the expected functionality.
app/client/packages/design-system/ads/src/Templates/IDEHeader/IDEHeaderTitle.tsx (2)
1-3: LGTM: Clean imports with proper relative paths
8-8: Verify component rename impact
Let's verify that all references to the old HeaderTitle component have been updated to use IDEHeaderTitle.
✅ Verification successful
Component rename has been properly implemented
The codebase shows that HeaderTitle is only used as an interface name and in file names, while all component usage correctly references IDEHeaderTitle. The component rename has been handled appropriately across the codebase.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for any remaining references to HeaderTitle
rg -l "HeaderTitle" --type ts --type tsx
# Search for usage of the new IDEHeaderTitle
rg -l "IDEHeaderTitle" --type ts --type tsx
Length of output: 140
Script:
#!/bin/bash
# Search for any remaining references to HeaderTitle without file type restrictions
rg -l "HeaderTitle"
# Search for usage of the new IDEHeaderTitle without file type restrictions
rg -l "IDEHeaderTitle"
Length of output: 822
Script:
#!/bin/bash
# Check the content of files that might still use HeaderTitle as a component name
rg "HeaderTitle" app/client/src/ee/pages/Editor/IDE/Header/useLibraryHeaderTitle.ts app/client/src/ce/pages/Editor/IDE/Header/useLibraryHeaderTitle.ts -C 2
# Check how HeaderTitle is used in the IDE Header index file
rg "HeaderTitle" app/client/src/pages/Editor/IDE/Header/index.tsx -C 2
Length of output: 2617
app/client/packages/design-system/ads/src/Templates/IDEHeader/HeaderSwitcher/HeaderSwitcher.styles.ts (1)
1-2: LGTM! Clean imports.
app/client/packages/design-system/ads/src/Templates/IDEHeader/index.ts (1)
16-19: LGTM! Documentation is appropriate for the styled component.
The component's purpose is clear and follows the established naming convention.
app/client/packages/design-system/ads/src/Templates/EntityExplorer/ListWithHeader.tsx (2)
1-4: LGTM! Clean and necessary imports.
1-38: Verify integration with existing IDE components.
The component implementation aligns well with the PR objectives of moving IDE components to ADS/Templates. However, let's verify its integration with existing IDE components.
✅ Verification successful
Component implementation is self-contained and properly structured
The component is a new addition to the design system's EntityExplorer templates, using the existing ListHeaderContainer styled component. No existing components need to be updated as this is a new abstraction that encapsulates the list header pattern. The implementation is clean and doesn't introduce any breaking changes to existing code.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for existing usage patterns of similar list headers in IDE components
# Search for similar list header patterns in IDE components
rg -l "ListHeader|EntityHeader" --type tsx
# Check for potential components that might need to be updated to use this new component
ast-grep --pattern 'const $_ = styled.$_`
$$$
overflow: auto;
$$$
header {
$$$
}
$$$
`'
Length of output: 178
Script:
#!/bin/bash
# Search for similar list header patterns in IDE components with correct file extension
rg -l "ListHeader|EntityHeader" --type ts --type tsx
# Look for styled components with header patterns
rg -U "const.*styled.*\{.*header.*\}" --type ts --type tsx
# Check for potential usage of this component
rg -l "ListWithHeader" --type ts --type tsx
# Check for similar existing components in design system
fd -e tsx -e ts . app/client/packages/design-system
Length of output: 45392
Script:
#!/bin/bash
# Search for potential usages of list headers in IDE components
rg -g "*.{ts,tsx}" "ListHeader|EntityHeader" app/client/packages/
# Look for similar components with header and list patterns
rg -g "*.{ts,tsx}" -l "header.*children.*overflow" app/client/packages/
# Check for any existing usage of ListWithHeader
rg -g "*.{ts,tsx}" "<ListWithHeader" app/client/packages/
Length of output: 1250
app/client/packages/design-system/ads/src/index.ts (1)
39-39: LGTM! Verify Templates directory structure
The new export follows the established pattern and aligns with the PR objectives.
Let's verify the Templates directory structure:
✅ Verification successful
Templates directory structure verified and properly organized
The Templates directory exists and contains the expected IDE Header components along with proper exports. The structure follows a clean organization with separate directories for IDEHeader and EntityExplorer, each having their own index files and related components.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify Templates directory structure and components
# Check if Templates directory exists and contains expected files
fd -t d "Templates" app/client/packages/design-system/ads/src
# Check for IDE Header related components in Templates
fd -t f . app/client/packages/design-system/ads/src/Templates -x echo "Found: {}"
# Verify exports from Templates/index.ts
rg "export \*" app/client/packages/design-system/ads/src/Templates/index.ts
Length of output: 1490
app/client/src/pages/Editor/commons/EditorWrapperContainer.tsx (2)
Line range hint 18-24: Verify constant value consistency after move.
The height calculation is critical for layout. Ensure the constant value remains the same after moving to ADS.
✅ Verification successful
Height calculation constant is consistently defined and used
The IDE_HEADER_HEIGHT constant is defined once in IDE/Structure/constants.ts with a value of 40px and is consistently used across multiple height calculations in the codebase. The moved code maintains the same calculation pattern.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Compare IDE_HEADER_HEIGHT values between old and new locations
# to ensure consistency and find all height calculations using this constant
# Find all height calculations using IDE_HEADER_HEIGHT
rg -p "calc\(.*IDE_HEADER_HEIGHT" "app/client/src"
# Find constant value definitions
rg -p "IDE_HEADER_HEIGHT.*=.*\d+" "app/client/src"
Length of output: 645
7-7: LGTM! Verify constant availability in new location.
The import path change aligns with moving IDE components to ADS.
app/client/packages/design-system/ads/src/Templates/IDEHeader/IDEHeader.tsx (4)
9-31: LGTM! Well-structured left section implementation
The component properly separates the logo section with a border and maintains consistent spacing.
33-45: LGTM! Clean center section implementation
The component maintains consistent layout properties with other sections.
78-80: LGTM! Clean compound component implementation
The component composition follows standard React patterns.
62-76: Verify overflow handling behavior
The component sets overflow: hidden which might clip important UI elements.
✅ Verification successful
Overflow hidden is appropriate for this header component
The overflow: hidden property is correctly applied here as this is a fixed-height header component (IDE_HEADER_HEIGHT). Since no absolute/fixed positioned elements or overflowing content were found in the usage patterns, this overflow behavior helps maintain a clean layout without causing any content clipping issues.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for any components that might be affected by overflow hidden
rg -l "header-right-section|header-left-section|header-center-section" | xargs rg "position:\s*absolute|position:\s*fixed"
Length of output: 124
Script:
#!/bin/bash
# Let's search for any children components rendered within IDEHeader
rg -A 5 "className=\"t--editor-header\""
# Also check for any styles applied to the header class
rg "t--editor-header" --type css --type scss
Length of output: 1107
Script:
#!/bin/bash
# Let's check what components are typically rendered inside IDEHeader
ast-grep --pattern 'const $_ = () => {
return (
<IDEHeader>
$$$
</IDEHeader>
)
}'
# Also check for any absolute/fixed positioned elements in the same files
rg -l "IDEHeader" | xargs rg "position:\s*(absolute|fixed)"
Length of output: 161
app/client/src/pages/Editor/JSEditor/styledComponents.ts (1)
9-9: LGTM! Verify consistent import usage across codebase.
The import change aligns with moving IDE components to ADS.
Let's verify that all files consistently use the new import path:
✅ Verification successful
Import path migration completed successfully
The verification shows that all imports of IDE_HEADER_HEIGHT are consistently using the new path @appsmith/ads. No instances of the old import path were found in the codebase.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for any remaining imports of IDE_HEADER_HEIGHT from the old path
# and confirm usage of new path
echo "Checking for old imports..."
rg "import.*IDE_HEADER_HEIGHT.*from.*\"IDE\""
echo "Verifying new imports..."
rg "import.*IDE_HEADER_HEIGHT.*from.*\"@appsmith/ads\""
Length of output: 1196
app/client/packages/design-system/ads/src/Templates/IDEHeader/HeaderSwitcher/IDEHeaderSwitcher.tsx (2)
19-31: LGTM! Clean implementation of forwardRef
The component follows React best practices with proper props destructuring and ref forwarding.
88-88: LGTM! Proper display name setup
The display name is correctly set for better debugging experience.
app/client/src/pages/Editor/IDE/EditorPane/PagesSection.tsx (5)
2-2: LGTM: Import changes align with ADS migration
The new imports from @appsmith/ads are consistent with the PR objective of moving components to ADS/Templates.
52-52: LGTM: Improved dependency array completeness
The addition of workspaceId and instanceId to the dependency array ensures proper reactivity when these values change.
59-63: LGTM: Proper list item structure implementation
The ListItemContainer wrapper provides consistent styling and structure for each page element.
66-85: LGTM: Well-structured context menu implementation
Good implementation with:
- Early permission check
- Proper memoization
- Clear prop passing
88-95: Verify the maxHeight constraint impact
While the ListWithHeader implementation looks good, please verify that the 300px maxHeight constraint:
- Doesn't cut off content unexpectedly
- Works well with different screen sizes
- Maintains good UX when many pages are present
✅ Verification successful
maxHeight constraint is properly implemented with scrolling support
The ListWithHeader component is correctly implemented with:
- Outer container with
overflow: hiddento maintain the maxHeight constraint - Inner content container with
overflow: autoto enable scrolling when content exceeds the height - Flex layout ensuring proper content distribution
The 300px constraint in PagesSection is safe to use as the component will gracefully handle overflow with scrolling.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for other instances of ListWithHeader to compare height constraints
rg -l 'ListWithHeader.*maxHeight'
Length of output: 35
Script:
#!/bin/bash
# Let's try a broader search for ListWithHeader usage patterns
rg -A 5 'ListWithHeader'
# Also search for the component definition to understand its props
ast-grep --pattern 'interface $_ {
$$$
maxHeight$_
$$$
}'
Length of output: 2438
Script:
#!/bin/bash
# Let's check the ListWithHeader component implementation and its usage in EntityExplorer
cat app/client/packages/design-system/ads/src/Templates/EntityExplorer/ListWithHeader.tsx
# Also search for any scroll-related styles or components used with ListWithHeader
rg -A 3 'overflow|scroll' app/client/packages/design-system/ads/src/Templates/EntityExplorer/ListWithHeader.tsx
Length of output: 1353
app/client/src/pages/Editor/IDE/Header/index.tsx (3)
16-17: LGTM: Import changes align with ADS migration
The new imports from @appsmith/ads align with the PR objective of moving IDE header components to ADS/Templates.
Line range hint 95-118: Verify EditorTitle functionality without title prop
The removal of the title prop from EditorTitle needs verification to ensure it can still display the correct title.
✅ Verification successful
EditorTitle component is self-sufficient without title prop
The EditorTitle component correctly retrieves the page title from Redux state using currentPage.pageName. The removal of the title prop is valid as the component manages its own title internally.
- Component fetches current page ID from Redux:
getCurrentPageId - Retrieves page details using selector:
getPageById - Uses
currentPage.pageNamefor display
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for EditorTitle usage and implementation
ast-grep --pattern 'class EditorTitle extends $_ {
$$$
}' || ast-grep --pattern 'function EditorTitle($_) {
$$$
}'
# Search for title prop usage in EditorTitle
rg -A 5 'title.*prop' ./app/client/src/pages/Editor/IDE/Header/EditorTitle*
Length of output: 142
Script:
#!/bin/bash
# Let's try a different approach to find EditorTitle implementation
rg -A 20 "const EditorTitle" ./app/client/src/pages/Editor/IDE/Header/
# Also search for any exports of EditorTitle
rg "export.*EditorTitle" ./app/client/src/pages/Editor/IDE/Header/
# Search for any imports of EditorTitle to understand its usage
rg "import.*EditorTitle" ./app/client/src/pages/Editor/
Length of output: 2138
221-222: Verify AppsmithLink integration in the header
The AppsmithLink component is now used as the logo in IDEHeader.Left. Ensure it maintains the correct navigation functionality.
✅ Verification successful
AppsmithLink integration looks correct and maintains navigation functionality
The component correctly uses the Link component from @appsmith/ads with proper navigation configuration:
- Uses StyledLink with
to={APPLICATIONS_URL}for navigation - Sets
target="_self"for in-app navigation - Includes tooltip and proper accessibility attributes
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for AppsmithLink implementation and usage
ast-grep --pattern 'export const AppsmithLink = $_' || ast-grep --pattern 'export function AppsmithLink($_)'
# Search for navigation-related props or hooks in AppsmithLink
rg -A 5 'useNavigate|useHistory|onClick' ./app/client/src/pages/Editor/AppsmithLink*
Length of output: 143059
Script:
#!/bin/bash
# Let's try a different approach to find the AppsmithLink component and its usage
# First find the file
fd AppsmithLink.tsx
# Then look at its content
rg -A 10 "export const AppsmithLink" app/client/src/pages/Editor/AppsmithLink.tsx
# Also check for any navigation-related imports
rg "^import.*navigate|^import.*history|^import.*Link" app/client/src/pages/Editor/AppsmithLink.tsx
# Look for StyledLink component definition
rg "const StyledLink" app/client/src/pages/Editor/AppsmithLink.tsx
Length of output: 748
app/client/packages/design-system/ads/src/Templates/EntityExplorer/ListWithHeader.tsx
Show resolved
Hide resolved
# Conflicts: # app/client/src/IDE/Components/HeaderDropdown.tsx
app/client/packages/design-system/ads/src/Templates/EntityExplorer/styles.ts
Outdated
Show resolved
Hide resolved
## 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.Sanity" ## Communication Should the DevRel and Marketing teams inform users about this change? - [ ] Yes - [x] No <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit ## Release Notes - **New Features** - Introduced `ListWithHeader` component for improved layout in the entity explorer. - Added `IDEHeader` component with subcomponents for better header organization. - Implemented `IDEHeaderSwitcher` for enhanced navigation options in the header. - Added styled components `ListItemContainer` and `ListHeaderContainer` for consistent design. - **Bug Fixes** - Updated import paths for `IDE_HEADER_HEIGHT` to ensure consistent usage across components. - **Documentation** - Added comprehensive documentation for the `IDEHeader` component to aid developers. - **Chores** - Consolidated import statements for cleaner code structure across various components. <!-- 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.Sanity"
🔍 Cypress test results
Tip
🟢 🟢 🟢 All cypress tests have passed! 🎉 🎉 🎉
Workflow run: https://github.com/appsmithorg/appsmith/actions/runs/12044915464
Commit: 873e6c8
Cypress dashboard.
Tags:
@tag.SanitySpec:
Wed, 27 Nov 2024 07:05:13 UTC
Communication
Should the DevRel and Marketing teams inform users about this change?
Summary by CodeRabbit
Release Notes
New Features
ListWithHeadercomponent for improved layout in the entity explorer.IDEHeadercomponent with subcomponents for better header organization.IDEHeaderSwitcherfor enhanced navigation options in the header.ListItemContainerandListHeaderContainerfor consistent design.Bug Fixes
IDE_HEADER_HEIGHTto ensure consistent usage across components.Documentation
IDEHeadercomponent to aid developers.Chores