fix: App title drop down close on interaction with other components. #36504
Conversation
saicharan-zemoso
requested review from
ApekshaBhosale and
sagar-qa007
as code owners
WalkthroughThis pull request enhances the Changes
Assessment against linked issues
Suggested labels
Suggested reviewers
Poem
📜 Recent review detailsConfiguration used: .coderabbit.yaml 📒 Files selected for processing (1)
🧰 Additional context used🔇 Additional comments (4)
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
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
Outside diff range and nitpick comments (1)
app/client/cypress/e2e/Regression/ClientSide/OtherUIFeatures/ApplicationDropdownClose_spec.ts (1)
1-2: Class, let's improve our import statements!Good job on importing the necessary modules, but we can make it even better:
- Let's use consistent import syntax. Instead of
require, use theimportstatement for the Omnibar locator.- We should follow camelCase naming convention for imported constants.
Here's how we can refactor these lines:
import homePage from "../../../../locators/HomePage"; import omnibarLocators from "../../../../locators/Omnibar.json";Remember, consistency is key in coding, just like in your homework assignments!
Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Files selected for processing (2)
- app/client/cypress/e2e/Regression/ClientSide/OtherUIFeatures/ApplicationDropdownClose_spec.ts (1 hunks)
- app/client/src/pages/Editor/EditorName/NavigationMenu.tsx (1 hunks)
Additional context used
Path-based instructions (1)
app/client/cypress/e2e/Regression/ClientSide/OtherUIFeatures/ApplicationDropdownClose_spec.ts (1)
Pattern
app/client/cypress/**/**.*: Review the following e2e test code written using the Cypress test library. Ensure that:
- Follow best practices for Cypress code and e2e automation.
- Avoid using cy.wait in code.
- Avoid using cy.pause in code.
- Avoid using agHelper.sleep().
- Use locator variables for locators and do not use plain strings.
- Use data-* attributes for selectors.
- Avoid Xpaths, Attributes and CSS path.
- Avoid selectors like .btn.submit or button[type=submit].
- Perform logins via API with LoginFromAPI.
- Perform logout via API with LogOutviaAPI.
- Perform signup via API with SignupFromAPI.
- Avoid using it.only.
- Avoid using after and aftereach in test cases.
- Use multiple assertions for expect statements.
- Avoid using strings for assertions.
- Do not use duplicate filenames even with different paths.
- Avoid using agHelper.Sleep, this.Sleep in any file in code.
Additional comments not posted (2)
app/client/src/pages/Editor/EditorName/NavigationMenu.tsx (1)
17-22: Well done, class! This change addresses our dropdown dilemma.Children, let's examine this wonderful addition to our
MenuContentcomponent. The newonInteractOutsideprop is like a vigilant hall monitor, ensuring our dropdown behaves properly when we interact with other parts of our application.This change directly solves the problem described in issue #28114, where our app title dropdown was being a bit stubborn and refusing to close when we opened the omnibar. Now, when you click outside or interact with other components, the dropdown will politely excuse itself and close.
Remember, class, in user interface design, it's important to ensure that our components play nicely with each other. This change is an excellent example of improving user experience through thoughtful interaction handling.
app/client/cypress/e2e/Regression/ClientSide/OtherUIFeatures/ApplicationDropdownClose_spec.ts (1)
4-5: Excellent job on your test descriptions!Your test suite and case descriptions are clear and informative. They provide a good overview of what's being tested, just like a well-written essay introduction. Keep up the good work!
app/client/cypress/e2e/Regression/ClientSide/OtherUIFeatures/ApplicationDropdownClose_spec.ts
Outdated
Show resolved
Hide resolved
app/client/cypress/e2e/Regression/ClientSide/OtherUIFeatures/ApplicationDropdownClose_spec.ts
Outdated
Show resolved
Hide resolved
app/client/cypress/e2e/Regression/ClientSide/OtherUIFeatures/ApplicationDropdownClose_spec.ts
Outdated
Show resolved
Hide resolved
|
This PR has not seen activitiy for a while. It will be closed in 7 days unless further activity is detected. |
|
/build-deploy-preview skip-tests=true |
|
Deploying Your Preview: https://github.com/appsmithorg/appsmith/actions/runs/11251120175. |
| <MenuContent width="214px"> | ||
| <MenuContent | ||
| width="214px" | ||
| onInteractOutside={() => { |
There was a problem hiding this comment.
This will create a new function on every re-render, possibly invalidating cache inside MenuContent. I believe, for that purpose, we have a lint rule for this. As a rule of thumb, if we pass an object to a component, that object should have a stable ref.
Could you please wrap this in a callback?
const handleInteractionOutside = useCallback(() => {
setIsPopoverOpen(false);
}, [setIsPopoverOpen]);
return (
<MenuContent
width="214px"
onInteractOutside={handleInteractionOutside}
|
Deploy-Preview-URL: https://ce-36504.dp.appsmith.com |
|
All tests are passing in shadow PR. |
Fixes #28114
fixed the app title drop down and it closes on interaction with other components
Before fixing the bug the omni bar and title dropdown overlap over each other:
after the fix the app title drop down closes on opening omni bar or any other component.
added the cypress test to visually confirm that the fix was working and did not add unit test as to test this we need to interact with other components or open omni bar while testing.
Summary by CodeRabbit
New Features
Bug Fixes