Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat: Updated new tab ui with search and load more #34981

Merged
merged 10 commits into from
Jul 23, 2024

Conversation

albinAppsmith
Copy link
Collaborator

@albinAppsmith albinAppsmith commented Jul 17, 2024

Description

Updated new tab ui with search, load more and updated titles.

Fixes #34809, #34530

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/10036807357
Commit: 2d37c01
Cypress dashboard.
Tags: @tag.All
Spec:


Mon, 22 Jul 2024 08:46:24 UTC

Communication

Should the DevRel and Marketing teams inform users about this change?

  • Yes
  • No

Summary by CodeRabbit

Summary by CodeRabbit

  • New Features

    • Added search functionality in the JS and Query Editor Panes to filter and display grouped items based on search terms.
    • Introduced a new component to render grouped lists with dynamic load more functionality.
  • Enhancements

    • Improved text and terminology for clearer user understanding in various modules.
    • Enhanced layout alignment by adjusting component heights to account for editor tabs.
  • Refactor

    • Improved type safety and refined logic in multiple functions for better performance and maintainability.
    • Replaced hardcoded values with constants for consistent and easier management.

@github-actions github-actions bot added IDE Navigation Issues/feature requests related to IDE navigation, and context switching IDE Pod Issues that new developers face while exploring the IDE Task A simple Todo labels Jul 17, 2024
Copy link
Contributor

coderabbitai bot commented Jul 17, 2024

Walkthrough

The updates primarily enhance the new tab UI by introducing a search functionality, allowing users to filter items efficiently. Implementing load-more features for groups with more than five items improves usability, while terminology updates and type safety enhancements contribute to overall code quality and clarity.

Changes

Files Modified Change Summary
messages.ts Updated text strings for functions: js_create_modules, queries_create_from_existing, queries_create_new, and queries_create_modules.
hooks.tsx Refactored imports, enhanced type safety for data in getListItems, and improved logic handling within the function.
Add.tsx, List.tsx (JS Module) Introduced search functionality with useState and SearchInput, updated imports, and refined item filtering and rendering logic.
Add.tsx, List.tsx (Query Module) Added search functionality, improved imports, and refined item filtering and rendering logic similar to the JS Module.
Group.tsx Introduced a new Group component for managing visibility and dynamic loading of grouped items.
constants.ts Defined EDITOR_TABS_HEIGHT constant for consistent height styling across components.
utils.ts Renamed fuzzySearchInFiles to fuzzySearchInObjectItems, modified its signature, and added parameters for flexible searching.
Container.tsx Replaced hardcoded height values with EDITOR_TABS_HEIGHT for consistent styling.
JSAddState.tsx, QueriesAddState.tsx Adjusted height of Flex components to align with editor tab heights for improved layout.

Sequence Diagram(s)

(Changes do not include significant modifications to control flow or new complex features that require a sequence diagram.)

Assessment against linked issues

Objective Addressed Explanation
Add search to new tab UI (#34809)
Add load more to individual groups if items > 5 (#34809)
Change titles as per design (#34809)

Poem

In the code's deep sea, a search was born,
To find the modules, on early morn.
With titles clear and items more,
The users now can deeply explore.
A constant height, for tabs up high,
And seamless views, as days go by.
Cheers to changes, let’s give it a try! 🎉


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?

Share
Tips

Chat

There are 3 ways to chat with CodeRabbit:

  • Review comments: Directly reply to a review comment made by CodeRabbit. Example:
    • I pushed a fix in commit <commit_id>.
    • Generate unit testing code for this file.
    • Open a follow-up GitHub issue for this discussion.
  • Files and specific lines of code (under the "Files changed" tab): Tag @coderabbitai in a new review comment at the desired location with your query. Examples:
    • @coderabbitai generate unit testing code for this file.
    • @coderabbitai modularize this function.
  • PR comments: Tag @coderabbitai in a new PR comment to ask questions about the PR branch. For the best results, please provide a very specific query, as very limited context is provided in this mode. Examples:
    • @coderabbitai generate interesting stats about this repository and render them as a table.
    • @coderabbitai show all the console.log statements in this repository.
    • @coderabbitai read src/utils.ts and generate unit testing code.
    • @coderabbitai read the files in the src/scheduler package and generate a class diagram using mermaid and a README in the markdown format.
    • @coderabbitai help me debug CodeRabbit configuration file.

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 as PR comments)

  • @coderabbitai pause to pause the reviews on a PR.
  • @coderabbitai resume to resume the paused reviews.
  • @coderabbitai review to trigger an incremental review. This is useful when automatic reviews are disabled for the repository.
  • @coderabbitai full review to do a full review from scratch and review all the files again.
  • @coderabbitai summary to regenerate the summary of the PR.
  • @coderabbitai resolve resolve all the CodeRabbit review comments.
  • @coderabbitai configuration to show the current CodeRabbit configuration for the repository.
  • @coderabbitai help to get help.

Additionally, you can add @coderabbitai ignore anywhere in the PR description to prevent this PR from being reviewed.

CodeRabbit Configuration File (.coderabbit.yaml)

  • You can programmatically configure CodeRabbit by adding a .coderabbit.yaml file to the root of your repository.
  • Please see the configuration documentation for more information.
  • If your editor has YAML language server enabled, you can add the path at the top of this file to enable auto-completion and validation: # yaml-language-server: $schema=https://coderabbit.ai/integrations/schema.v2.json

Documentation and Community

  • Visit our Documentation for detailed information on how to use CodeRabbit.
  • Join our Discord Community to get help, request features, and share feedback.
  • Follow us on X/Twitter for updates and announcements.

@github-actions github-actions bot added the Enhancement New feature or request label Jul 17, 2024
Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 1

Outside diff range, codebase verification and nitpick comments (1)
app/client/src/pages/Editor/IDE/EditorPane/components/Group.tsx (1)

49-52: Consider revising the conditional logic for special cases.

The logic to handle special cases for "Datasources" group seems a bit unclear. It's important to document why this specific case is needed or consider a more generic approach that could handle similar cases without hardcoding group names.

- if (hasMoreItems && groupTitle === "Datasources") {
+ // TODO: Review the necessity of this condition or make it more generic
Review details

Configuration used: .coderabbit.yaml
Review profile: CHILL

Commits

Files that changed from the base of the PR and between e58b10d and 6ddf1c8.

Files selected for processing (16)
  • app/client/src/ce/constants/messages.ts (1 hunks)
  • app/client/src/ce/pages/Editor/IDE/EditorPane/Query/hooks.tsx (4 hunks)
  • app/client/src/pages/Editor/IDE/EditorPane/JS/Add.tsx (4 hunks)
  • app/client/src/pages/Editor/IDE/EditorPane/JS/List.tsx (4 hunks)
  • app/client/src/pages/Editor/IDE/EditorPane/Query/Add.tsx (3 hunks)
  • app/client/src/pages/Editor/IDE/EditorPane/Query/List.tsx (4 hunks)
  • app/client/src/pages/Editor/IDE/EditorPane/components/EmptySearchResult.tsx (1 hunks)
  • app/client/src/pages/Editor/IDE/EditorPane/components/Group.tsx (1 hunks)
  • app/client/src/pages/Editor/IDE/EditorPane/components/GroupedList.tsx (1 hunks)
  • app/client/src/pages/Editor/IDE/EditorPane/components/SegmentAddHeader.tsx (1 hunks)
  • app/client/src/pages/Editor/IDE/EditorPane/components/constants.ts (1 hunks)
  • app/client/src/pages/Editor/IDE/EditorPane/components/types.ts (1 hunks)
  • app/client/src/pages/Editor/IDE/EditorPane/fuzzySearchInFiles.test.ts (3 hunks)
  • app/client/src/pages/Editor/IDE/EditorPane/utils.ts (2 hunks)
  • app/client/src/pages/Editor/JSEditor/JSAddState.tsx (1 hunks)
  • app/client/src/pages/Editor/QueryEditor/QueriesAddState.tsx (1 hunks)
Files skipped from review due to trivial changes (1)
  • app/client/src/pages/Editor/IDE/EditorPane/components/constants.ts
Additional comments not posted (31)
app/client/src/pages/Editor/IDE/EditorPane/components/types.ts (1)

3-7: Interface Definition Review: GroupedListProps

This interface is well-defined with clear property types. The use of ListItemProps ensures type safety for items, which is crucial for consistency across the UI components that will utilize this interface.

  • Suggestion: If groupTitle is expected to be commonly used, consider providing a default value or handling cases where it might be undefined in the components that consume this interface.
  • Suggestion: Ensure that className is utilized effectively in the components for styling purposes, as it's a required property.
app/client/src/pages/Editor/JSEditor/JSAddState.tsx (1)

7-7: Height Adjustment in Flex Component

The adjustment to the Flex component's height using calc(100% - 32px) is a practical solution to ensure that the UI does not overflow and maintains the intended layout. This change aligns with the PR's objective to refine the UI.

  • Verification: Confirm that the 32px reduction in height does not negatively impact the usability or visibility of elements within the Flex container across different screen sizes and resolutions.
app/client/src/pages/Editor/QueryEditor/QueriesAddState.tsx (1)

7-7: Consistent Height Adjustment Across Components

Applying the same height adjustment (calc(100% - 32px)) in QueriesAddState as in JSAddState demonstrates a consistent approach to UI design across different editor states. This ensures a uniform user experience.

  • Verification: It's important to test this change in the context of the overall application to ensure it integrates well without causing any layout shifts or usability issues.
Verification successful

Let's correct the command to search for .tsx files where the QueriesAddState and JSAddState components might be used.


Alright, let's refine our approach. Instead of specifying the file type, we'll search for the component names directly across all files. This will help us identify where these components are used and verify the consistency in UI adjustments.


Consistent Height Adjustment Across Components Verified

The height adjustment (calc(100% - 32px)) applied in both QueriesAddState and JSAddState components demonstrates a consistent approach to UI design across different editor states. This ensures a uniform user experience.

  • Locations:
    • app/client/src/pages/Editor/QueryEditor/QueriesAddState.tsx
    • app/client/src/pages/Editor/JSEditor/JSAddState.tsx
    • app/client/src/ce/pages/Editor/IDE/MainPane/useRoutes.ts
Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Description: Verify consistent UI adjustments across components.

# Test: Search for usage of `QueriesAddState` and `JSAddState` in the application. Expect: Consistent layout adjustments.
rg --type javascript -A 5 $'QueriesAddState|JSAddState'

Length of output: 91


Script:

#!/bin/bash
# Description: Verify consistent UI adjustments across components.

# Test: Search for usage of `QueriesAddState` and `JSAddState` in the application. Expect: Consistent layout adjustments.
rg --type tsx -A 5 'QueriesAddState|JSAddState'

Length of output: 77


Script:

#!/bin/bash
# Description: Verify consistent UI adjustments across components.

# Test: Search for usage of `QueriesAddState` and `JSAddState` in the application. Expect: Consistent layout adjustments.
rg 'QueriesAddState|JSAddState' -A 5

Length of output: 2923

app/client/src/pages/Editor/IDE/EditorPane/components/EmptySearchResult.tsx (1)

5-17: Implementation of EmptySearchResult Component

This component effectively handles the display of empty search results by using dynamic messaging based on the type prop. The use of centralized message management (createMessage) enhances maintainability and localization efforts.

  • Suggestion: Add PropTypes or TypeScript definitions for the type prop to enforce type safety and provide clearer documentation for other developers.
  • Verification: Ensure that the empty search result message is appropriately styled and readable across different devices and themes.
app/client/src/pages/Editor/IDE/EditorPane/components/GroupedList.tsx (5)

2-2: New import of FlexProps type.

Adding a specific type import for FlexProps ensures better type safety and clarity in the component's props usage. This is a good practice.


5-5: New import of GroupedListProps type.

Importing GroupedListProps from a local types file is a good practice for maintaining modularity and clarity in type definitions.


9-9: Change in groups prop type from GroupedListProps to GroupedListProps[].

This change aligns with the requirement to handle multiple groups, supporting the new "load more" functionality. It's a necessary update for the feature enhancement.


13-17: Introduction of StyledFlex styled component.

The new styled component StyledFlex is used to apply specific styles to the last child of the group list. This is a clean way to handle CSS in React components using styled-components. The specific style of removing the border from the last child helps in better UI presentation without extra borders hanging at the bottom.


21-31: Usage of StyledFlex and mapping of Group components.

The refactoring to use StyledFlex and the Group component for each group item is a clean and modular approach. It enhances readability and maintainability of the component. Ensure that the Group component is well-tested, especially since it now plays a crucial role in rendering group items.

app/client/src/pages/Editor/IDE/EditorPane/utils.ts (2)

2-2: Replacement of specific import with lodash/get.

Replacing a more specific import with a utility function from lodash can be beneficial for handling deeply nested properties safely. This change likely supports the new dynamic functionalities like search and load more by providing a flexible way to access properties.


13-28: Renaming and redefining fuzzySearchInFiles to fuzzySearchInObjectItems.

The renaming and expansion of the function's capabilities to handle a generic type and additional parameters for more flexible searching is a strong improvement. It supports the new search functionality robustly. However, ensure that all references to the old function name are updated across the project to avoid breaking changes.

Verification successful

Let's correct the file type in the search command and re-run the script to verify if all references to the old function name fuzzySearchInFiles have been updated.


Let's update the file type to ts and re-run the script to ensure all references to the old function name fuzzySearchInFiles have been updated.


All references to fuzzySearchInFiles have been updated.

The search did not find any occurrences of the old function name fuzzySearchInFiles, indicating that all references have been successfully updated to fuzzySearchInObjectItems.

  • No further action is needed.
Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Description: Verify that all references to the old function `fuzzySearchInFiles` have been updated.

# Test: Search for the old function name usage. Expect: No occurrences of the old function name.
rg --type python -A 5 $'fuzzySearchInFiles'

Length of output: 73


Script:

#!/bin/bash
# Description: Verify that all references to the old function `fuzzySearchInFiles` have been updated.

# Test: Search for the old function name usage. Expect: No occurrences of the old function name.
rg --type typescript -A 5 $'fuzzySearchInFiles'

Length of output: 81


Script:

#!/bin/bash
# Description: Verify that all references to the old function `fuzzySearchInFiles` have been updated.

# Test: Search for the old function name usage. Expect: No occurrences of the old function name.
rg --type ts -A 5 $'fuzzySearchInFiles'

Length of output: 38

app/client/src/pages/Editor/IDE/EditorPane/components/SegmentAddHeader.tsx (1)

Line range hint 1-1: General review of SegmentAddHeader.tsx.

The file shows adjustments in styling properties to better align with design changes. It's important to ensure these changes are consistent with the design specs provided in the Zeplin link. It would be beneficial to visually verify these changes in a development environment or through screenshots to ensure they meet the expected UI updates.

Verification successful

Let's gather more information about the content of SegmentAddHeader.tsx to verify the review comment accurately.


Please verify the import statements and the JSX return block for correctness and consistency:

  • Import Statements:

    • Ensure all necessary modules are imported correctly.
    • Verify that Button, Flex, and Text components are correctly imported from design-system.
    • Confirm that createMessage and useSelector are imported from the right modules.
  • JSX Return Block:

    • Check the conditional rendering logic based on isSideBySideEnabled.
    • Ensure the Flex component's backgroundColor prop correctly switches between two colors.
    • Verify the Text component's usage and the dynamic message rendering.
    • Confirm the conditional rendering of the Button component and its props.

Please review these aspects to ensure the code changes align with the design specifications and functionality requirements.

Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Description: Verify that the changes match the design specifications.

# Test: Compare the implemented styles with the Zeplin design specifications.
echo "Please manually verify the UI changes with the Zeplin design specifications."

Length of output: 162


Script:

#!/bin/bash
# Display the content of SegmentAddHeader.tsx to understand the changes made.
cat $(fd SegmentAddHeader.tsx)

Length of output: 1258

app/client/src/pages/Editor/IDE/EditorPane/fuzzySearchInFiles.test.ts (1)

2-2: Update in test file to reflect function renaming.

Updating the import and test cases to reflect the renaming of fuzzySearchInFiles to fuzzySearchInObjectItems is crucial to maintain accurate and relevant tests. This change ensures that the tests align with the updated functionality of the utility functions.

app/client/src/pages/Editor/IDE/EditorPane/Query/Add.tsx (4)

1-2: Import useState properly for better code clarity.

You've imported useState directly in the React import line, which is a common practice and makes the code cleaner by reducing the number of import lines. Well done!


13-15: Ensure utility and component imports are correctly placed and used.

The imports for fuzzySearchInObjectItems, GroupedListProps, and EmptySearchResult are correctly placed. Make sure these utilities and components are used appropriately in the codebase to avoid any runtime issues.


18-32: Proper use of React state and utility functions.

You've used useState to manage the searchTerm state, which is appropriate for handling input changes in React. The use of fuzzySearchInObjectItems to filter items based on the search term is a good application of utility functions to keep the component logic clean.


53-57: Implement search functionality with dynamic rendering based on conditions.

The implementation of the search input and conditional rendering of GroupedList and EmptySearchResult based on localGroups.length and searchTerm is well done. This ensures that the UI dynamically responds to user input, providing a good user experience.

app/client/src/pages/Editor/IDE/EditorPane/components/Group.tsx (1)

27-47: Implementation of 'Load more' functionality with proper state management.

The use of useState to manage visibleItems and the implementation of handleLoadMore function to increase the number of visible items are correctly done. This pattern is effective for "load more" functionalities.

app/client/src/pages/Editor/IDE/EditorPane/JS/Add.tsx (2)

1-5: Correct import and use of React hooks and components.

The import of useState along with useCallback is correctly done. This setup is essential for managing state and memoizing functions, which can help in optimizing performance.


Line range hint 23-61: Proper implementation of search and dynamic content rendering.

The use of useState for searchTerm, the application of fuzzySearchInObjectItems for filtering, and the dynamic rendering of GroupedList or EmptySearchResult based on the search results are all correctly implemented. This dynamic approach enhances user interaction by immediately reflecting the changes based on the search input.

app/client/src/pages/Editor/IDE/EditorPane/Query/List.tsx (3)

Line range hint 14-24: Proper import and use of utility functions and components.

The import of fuzzySearchInObjectItems and EmptySearchResult is correctly done. These are crucial for implementing the enhanced search functionality and handling empty states.


34-37: Correct application of the updated search utility function.

The replacement of fuzzySearchInFiles with fuzzySearchInObjectItems is a good update, as it likely offers a more tailored search functionality for the object items used in this context.


103-103: Proper handling of empty search results.

The use of EmptySearchResult to display a message when no items match the search term is a user-friendly approach. This ensures that users are not left wondering why no results are shown.

app/client/src/pages/Editor/IDE/EditorPane/JS/List.tsx (5)

6-6: Import type verification

The import of EditorSegmentList as a type is a good practice, ensuring that TypeScript only includes it during compilation for type checking and not in the final JavaScript bundle. This keeps the bundle size smaller.


23-23: Function Renaming: Clarity and Consistency

Renaming fuzzySearchInFiles to fuzzySearchInObjectItems enhances clarity, indicating that the function can be used more generically, not just for files. This change aligns well with the principle of clear naming in coding.


24-24: Component Replacement for Empty Results

Replacing the constant EDITOR_PANE_TEXTS.empty_search_result with a component EmptySearchResult is a significant improvement. This change likely enhances the UI by providing a more robust and customizable way to handle empty search results, possibly including additional styling or behavior.


48-51: Usage of Renamed Function

Here, the renamed function fuzzySearchInObjectItems is used correctly. The generic type <EditorSegmentList> is specified, which should ensure that TypeScript enforces the correct structure on the items being searched. This is a good application of generics for type safety.


119-119: Conditional Rendering Based on Search Results

The conditional rendering logic here checks if there are no files after a search and displays an appropriate message. This is a user-friendly feature, ensuring that users are informed when there are no search results. The implementation is straightforward and effective.

app/client/src/ce/pages/Editor/IDE/EditorPane/Query/hooks.tsx (3)

1-1: Import Cleanup

Removing the default import of React since it's no longer necessary with the newer JSX Transform in React 17+. This cleanup avoids unnecessary imports and aligns with modern React practices.


36-36: Type Import Enhancement

Changing the import to specify only ListItemProps as a type import is a good practice. It makes the import statement clearer and potentially reduces the impact on the bundle size since type imports are erased during compilation.


Line range hint 185-204: Function Refactoring: Enhanced Type Safety and Logic Simplification

Refactoring getListItems to use ActionOperation[] instead of any[] improves type safety, ensuring that the function can only be called with the correct data type. The logic inside the function has been streamlined to handle properties of fileOperation more effectively, which should make the function both easier to understand and more robust.

Comment on lines +2315 to +2318
js_create_modules: () => "JS modules (Beta)",
queries_create_from_existing: () => "Datasources",
queries_create_new: () => "Quick actions",
queries_create_modules: () => "Query modules (Beta)",
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Consider using template literals for consistency and future extensibility.

The functions returning strings in this file use regular strings. For consistency with other parts of the codebase that might use template literals and to facilitate easier modifications in the future (like introducing variables into the strings), consider using template literals here as well.

-  js_create_modules: () => "JS modules (Beta)",
-  queries_create_from_existing: () => "Datasources",
-  queries_create_new: () => "Quick actions",
-  queries_create_modules: () => "Query modules (Beta)",
+  js_create_modules: () => `JS modules (Beta)`,
+  queries_create_from_existing: () => `Datasources`,
+  queries_create_new: () => `Quick actions`,
+  queries_create_modules: () => `Query modules (Beta)`,
Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
js_create_modules: () => "JS modules (Beta)",
queries_create_from_existing: () => "Datasources",
queries_create_new: () => "Quick actions",
queries_create_modules: () => "Query modules (Beta)",
js_create_modules: () => `JS modules (Beta)`,
queries_create_from_existing: () => `Datasources`,
queries_create_new: () => `Quick actions`,
queries_create_modules: () => `Query modules (Beta)`,

@albinAppsmith albinAppsmith added the ok-to-test Required label for CI label Jul 17, 2024
@albinAppsmith
Copy link
Collaborator Author

/build-deploy-preview skip-test=true

Copy link

Deploying Your Preview: https://github.com/appsmithorg/appsmith/actions/runs/9969839075.
Workflow: On demand build Docker image and deploy preview.
skip-tests: . env: .
PR: 34981.
recreate: .

Copy link

Deploy-Preview-URL: https://ce-34981.dp.appsmith.com

descriptionType: "inline",
onClick: onCreateItemClick.bind(null, data),
wrapperClassName: createAddClassName(title),
} as ListItemProps;
};

const groups = groupedJsOperations.map((op) => ({
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is not required by any means, but I think if you destructure function arguments code become more concise and readable.

const groups = groupedJsOperations.map(({title, className, operations}) => ({
    groupTitle: title,
    className: className,
    items: operations.map(getListItems),
  }));

>
{createMessage(EDITOR_PANE_TEXTS.empty_search_result, "JS")}
</Text>
<EmptySearchResult type="JS object" />
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should this be imported from messages instead of being a string literal?

const [visibleItems, setVisibleItems] = useState<number>(
DEFAULT_GROUP_LIST_SIZE,
);
const { className, groupTitle } = group;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

const { className, groupTitle, groupItems } = group;
const items = groupItems.slice(0, visibleItems);
const hasMoreItems = groupItems.length > visibleItems;

const handleLoadMore = () => {
    setVisibleItemsCount(groupItems.length);
};

`;

const Group: React.FC<GroupProps> = ({ group }) => {
const [visibleItems, setVisibleItems] = useState<number>(
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I suggest renaming visibleItems to visibleItemsCount to reflect the real intent.

const [visibleItemsCount, setVisibleItemsCount] = useState<number>(

};

if (hasMoreItems) {
items.push({
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You should never mutate objects directly in React, as it can lead to unpredictable results. This can be fixed by moving items composition into a useMemo.

const items = useMemo(() => {
    const items = groupItems.slice(0, visibleItemsCount);
    const hasMoreItems = groupItems.length > visibleItemsCount;

    if (hasMoreItems) {
        items.push({
            title: "Load more...",
            description: "",
            descriptionType: "inline",
            onClick: handleLoadMore,
            className: "ds-load-more",
        });

         if (groupTitle === "Datasources") {
            items.push(groupItems[groupItems.length - 1]);
        }
    }

    return items;
}, [groupItems, visibleItemsCount, handleLoadMore, groupTitle]);

}

// TODO: try to avoid this
if (hasMoreItems && groupTitle === "Datasources") {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ideally string Datasources should be in a constant.

if (searchStr && searchStr !== "") {
const newFiles = files
.map((group) => {
const fuse = new Fuse(group.items, FUSE_OPTIONS);
.map((group: any) => {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

PLease avoid using any, we're expecting a certain shape here, so it should be typed properly.

.map((group) => {
const fuse = new Fuse(group.items, FUSE_OPTIONS);
.map((group: any) => {
const items = get(group, itemsKey);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why not just group[itemsKey]?

@@ -4,7 +4,7 @@ import AddJS from "pages/Editor/IDE/EditorPane/JS/Add";

const JSAddState = () => {
return (
<Flex height="100%" justifyContent="center">
<Flex height="calc(100% - 32px)" justifyContent="center">
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

To be clear with your intents 32px should be moved to a constant, so it becomes clear as to what we're making room for.

Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 0

Review details

Configuration used: .coderabbit.yaml
Review profile: CHILL

Commits

Files that changed from the base of the PR and between 6ddf1c8 and 6c3f2ed.

Files selected for processing (12)
  • app/client/src/ce/constants/messages.ts (1 hunks)
  • app/client/src/ce/pages/Editor/IDE/EditorPane/Query/hooks.tsx (4 hunks)
  • app/client/src/pages/Editor/IDE/EditorPane/JS/Add.tsx (4 hunks)
  • app/client/src/pages/Editor/IDE/EditorPane/JS/List.tsx (4 hunks)
  • app/client/src/pages/Editor/IDE/EditorPane/Query/Add.tsx (3 hunks)
  • app/client/src/pages/Editor/IDE/EditorPane/Query/List.tsx (4 hunks)
  • app/client/src/pages/Editor/IDE/EditorPane/components/Group.tsx (1 hunks)
  • app/client/src/pages/Editor/IDE/EditorPane/constants.ts (1 hunks)
  • app/client/src/pages/Editor/IDE/EditorPane/utils.ts (2 hunks)
  • app/client/src/pages/Editor/IDE/EditorTabs/Container.tsx (2 hunks)
  • app/client/src/pages/Editor/JSEditor/JSAddState.tsx (1 hunks)
  • app/client/src/pages/Editor/QueryEditor/QueriesAddState.tsx (1 hunks)
Files skipped from review due to trivial changes (1)
  • app/client/src/pages/Editor/IDE/EditorPane/constants.ts
Files skipped from review as they are similar to previous changes (10)
  • app/client/src/ce/constants/messages.ts
  • app/client/src/ce/pages/Editor/IDE/EditorPane/Query/hooks.tsx
  • app/client/src/pages/Editor/IDE/EditorPane/JS/Add.tsx
  • app/client/src/pages/Editor/IDE/EditorPane/JS/List.tsx
  • app/client/src/pages/Editor/IDE/EditorPane/Query/Add.tsx
  • app/client/src/pages/Editor/IDE/EditorPane/Query/List.tsx
  • app/client/src/pages/Editor/IDE/EditorPane/components/Group.tsx
  • app/client/src/pages/Editor/IDE/EditorPane/utils.ts
  • app/client/src/pages/Editor/JSEditor/JSAddState.tsx
  • app/client/src/pages/Editor/QueryEditor/QueriesAddState.tsx
Additional comments not posted (2)
app/client/src/pages/Editor/IDE/EditorTabs/Container.tsx (2)

4-4: Good use of centralized constants for UI dimensions.

Importing EDITOR_TABS_HEIGHT from "../EditorPane/constants" is a good practice as it helps maintain consistency in UI dimensions across the application.


14-15: Consistent application of EDITOR_TABS_HEIGHT for styling.

Using the EDITOR_TABS_HEIGHT for both maxHeight and minHeight ensures that the tabs container has a fixed height, which can contribute to a more predictable and stable UI layout.

@albinAppsmith albinAppsmith added ok-to-test Required label for CI and removed ok-to-test Required label for CI labels Jul 17, 2024
@albinAppsmith albinAppsmith added ok-to-test Required label for CI and removed ok-to-test Required label for CI labels Jul 22, 2024
@albinAppsmith albinAppsmith removed the ok-to-test Required label for CI label Jul 22, 2024
@albinAppsmith albinAppsmith added the ok-to-test Required label for CI label Jul 22, 2024
Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 0

Review details

Configuration used: .coderabbit.yaml
Review profile: CHILL

Commits

Files that changed from the base of the PR and between 6c3f2ed and 2d37c01.

Files selected for processing (3)
  • app/client/src/pages/Editor/IDE/EditorPane/JS/List.tsx (4 hunks)
  • app/client/src/pages/Editor/IDE/EditorPane/Query/List.tsx (4 hunks)
  • app/client/src/pages/Editor/IDE/EditorPane/components/Group.tsx (1 hunks)
Files skipped from review as they are similar to previous changes (3)
  • app/client/src/pages/Editor/IDE/EditorPane/JS/List.tsx
  • app/client/src/pages/Editor/IDE/EditorPane/Query/List.tsx
  • app/client/src/pages/Editor/IDE/EditorPane/components/Group.tsx

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Enhancement New feature or request IDE Navigation Issues/feature requests related to IDE navigation, and context switching IDE Pod Issues that new developers face while exploring the IDE ok-to-test Required label for CI Task A simple Todo
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[Task] - Implement search inside new tab
3 participants