fix: clicking on + in list view does not create a new query#36467
fix: clicking on + in list view does not create a new query#36467hetunandu merged 3 commits intoappsmithorg:releasefrom a6hishekpandey:fix/36066-clicking-on-plus-in-list-view-does-not-create-new-query
Conversation
WalkthroughThe pull request introduces several changes to manage the active state of a list view within the IDE. It adds a new action creator, action type, and selector to facilitate this state management using Redux. Additionally, modifications are made to existing hooks and components to utilize the new state management approach, enhancing the responsiveness of the application interface. Changes
Assessment against linked issues
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
|
…-list-view-does-not-create-new-query
|
@albinAppsmith can you please review the PR whenever you have sometime, and let me know if you have any suggestions on the same |
|
@a6hishekpandey I'm looking into this :) |
|
@albinAppsmith okay, thankyou |
|
@a6hishekpandey, I’ve identified an issue in the enterprise edition related to this PR. I will review and address it before proceeding with the merge. |
|
@a6hishekpandey, could you please apply the same changes made in Thank you. |
@albinAppsmith, we don't need those changes in JS as we are generating entire object with a default name so whenever we click on the plus from listview it will add a new js object and make that tab active, this problem will occur only in the case of QUERY as we have to select one type (ex: REST API) after the add button clicked. |
@a6hishekpandey the behaviour you explained is different in enterprise version and if you check the else part in |
okay, sure I will go through it and make the required changes. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (1)
app/client/src/ce/pages/Editor/IDE/EditorPane/JS/hooks.ts (1)
30-54: Excellent work on updating theuseJSAddhook!You've made some smart changes here:
- Adding the
ideViewModeselector.- Implementing the new condition for split screen mode.
- Updating the dependency array.
However, let's make one small improvement to enhance readability:
Consider extracting the new condition into a separate function. This will make the code more readable and easier to test. Here's an example:
const handleSplitScreenMode = () => { if (ideViewMode === EditorViewMode.SplitScreen) { dispatch(setListViewActiveState(false)); } }; // Then in openAddJS: if (currentEntityInfo.entity === FocusEntity.JS_OBJECT_ADD) { handleSplitScreenMode(); return; }This change will make your code even more organized and easier to understand. Keep up the great work!
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (1)
- app/client/src/ce/pages/Editor/IDE/EditorPane/JS/hooks.ts (2 hunks)
🔇 Additional comments not posted (2)
app/client/src/ce/pages/Editor/IDE/EditorPane/JS/hooks.ts (2)
18-20: Well done, class! These new imports are spot on.You've correctly added the necessary imports for the new functionality. Remember, good code organization starts with proper imports!
30-54: Let's review our lesson plan, shall we?Class, we've made some changes to address the '+' icon functionality in the list view. However, I have a few questions:
- The PR objectives mention QUERY_ADD mode, but this file handles JS object creation. Can you explain how this change aligns with the original issue?
- In the PR comments, it was mentioned that changes weren't necessary for JS hooks. Yet, we've implemented similar changes here. Could you clarify the reasoning behind this decision?
- How does this change interact with the enterprise version mentioned in the comments?
Let's discuss these points to ensure we're on the right track with our lesson plan.
|
@albinAppsmith, I did the required changes in openAddJS function as well, can you please check whenever you will have sometime? |
|
All tests passed in both CE and EE. CE: #36526 |
|
@albinAppsmith and @hetunandu, I had fixed one more issue but closed it as no one looked into it and was not sure if the fix I did was to be did like that, If someone can help me with the PR I can reopen it #36367. Please have a look if you guys have sometime. |
|
@a6hishekpandey This problem needs to be discussed internally before getting into that solution. I will get back to you on this. |
@albinAppsmith okay, sure |
a6hishekpandey
deleted the
fix/36066-clicking-on-plus-in-list-view-does-not-create-new-query
branch
|
@a6hishekpandey This fix involves some internal discussions and design decisions, so it's best to set it aside for now. Feel free to explore other issues from our Inviting Contribution list. Happy contributing 🙂 |
|
@albinAppsmith okay, thanks for the update. |
3 participants
Description
This change adds a new state in ideReducer 'isListViewActive' to verify the active status of the list view on split screen mode. I am updating isListViewActive state to false when we click on
+icon and are already in QUERY_ADD mode, to close the list view and switch back to the new query tab.Fixes #36066
or
Fixes
Issue URLWarning
If no issue exists, please create an issue first, and check with the maintainers if the issue is valid.
Automation
/ok-to-test tags=""
🔍 Cypress test results
Caution
If you modify the content in this section, you are likely to disrupt the CI result for your PR.
Communication
Should the DevRel and Marketing teams inform users about this change?
Summary by CodeRabbit
New Features
Improvements
useQueryAddanduseJSAddhooks to respond to the IDE's view mode, improving functionality when adding queries.Bug Fixes