fix: Added height to new query button menu#29729
Conversation
WalkthroughThe modification made to the 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 X ? TipsChat with CodeRabbit Bot (
|
github-actions
bot
added
Bug
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
| <MenuContent | ||
| align={"end"} | ||
| data-testId={"t--page-selection"} | ||
| height={"350px"} | ||
| side={"bottom"} | ||
| > |
There was a problem hiding this comment.
The addition of a fixed height to the MenuContent component is implemented as intended to address the dropdown issue. However, consider using a constant for the height value to improve maintainability and make it easier to adjust in the future if needed.
- height={"350px"}
+ const MENU_CONTENT_HEIGHT = "350px";
+ height={MENU_CONTENT_HEIGHT}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.
| <MenuContent | |
| align={"end"} | |
| data-testId={"t--page-selection"} | |
| height={"350px"} | |
| side={"bottom"} | |
| > | |
| <MenuContent | |
| align={"end"} | |
| data-testId={"t--page-selection"} | |
| const MENU_CONTENT_HEIGHT = "350px"; | |
| height={MENU_CONTENT_HEIGHT} | |
| side={"bottom"} | |
| > |
|
/ok-to-test |
|
/build-deploy-preview skip-tests=true |
|
Tests running at: https://github.com/appsmithorg/appsmith/actions/runs/7259268429. |
|
Deploying Your Preview: https://github.com/appsmithorg/appsmith/actions/runs/7259271046. |
|
Deploy-Preview-URL: https://ce-29729.dp.appsmith.com |
|
Workflow run: https://github.com/appsmithorg/appsmith/actions/runs/7259268429. |
| <MenuContent | ||
| align={"end"} | ||
| data-testId={"t--page-selection"} | ||
| height={pages.length <= 4 ? "fit-content" : "186px"} |
There was a problem hiding this comment.
The fixed height for the MenuContent component is set conditionally based on the number of pages. However, the height "186px" seems arbitrary. It would be beneficial to ensure this value aligns with the design system and consider using a constant for maintainability.
- height={pages.length <= 4 ? "fit-content" : "186px"}
+ const MENU_MAX_HEIGHT = "186px"; // This should be defined in a constants file or theme settings
+ height={pages.length <= 4 ? "fit-content" : MENU_MAX_HEIGHT}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.
| height={pages.length <= 4 ? "fit-content" : "186px"} | |
| const MENU_MAX_HEIGHT = "186px"; // This should be defined in a constants file or theme settings | |
| height={pages.length <= 4 ? "fit-content" : MENU_MAX_HEIGHT} |
Consider adding a comment explaining why the height is set to "fit-content" when there are 4 or fewer pages and "186px" otherwise. This will help future maintainers understand the reasoning behind these specific values.
If manual testing has not been mentioned, recommend adding a test case to ensure that the dropdown behaves as expected with different numbers of pages, especially at the boundary condition of 4 pages.
|
/ok-to-test |
|
/build-deploy-preview skip-tests=true |
|
Tests running at: https://github.com/appsmithorg/appsmith/actions/runs/7261112455. |
|
Deploying Your Preview: https://github.com/appsmithorg/appsmith/actions/runs/7261113319. |
|
Deploy-Preview-URL: https://ce-29729.dp.appsmith.com |
|
Workflow run: https://github.com/appsmithorg/appsmith/actions/runs/7261112455. |
…o new-query-menu-height-issue
|
/ok-to-test |
|
Tests running at: https://github.com/appsmithorg/appsmith/actions/runs/7270926741. |
|
Workflow run: https://github.com/appsmithorg/appsmith/actions/runs/7270926741. |
## Description This PR adds a fixed height to the new query button in datasource config page. #### PR fixes following issue(s) Fixes #29609 #### Type of change - Bug fix (non-breaking change which fixes an issue) ## Testing > #### How Has This Been Tested? > Please describe the tests that you ran to verify your changes. Also list any relevant details for your test configuration. > Delete anything that is not relevant - [ ] Manual - [ ] JUnit - [ ] Jest - [ ] Cypress > > #### Test Plan > Add Testsmith test cases links that relate to this PR > > #### Issues raised during DP testing > Link issues raised during DP testing for better visiblity and tracking (copy link from comments dropped on this PR) > > > ## Checklist: #### Dev activity - [x] My code follows the style guidelines of this project - [x] I have performed a self-review of my own code - [ ] I have commented my code, particularly in hard-to-understand areas - [ ] I have made corresponding changes to the documentation - [x] My changes generate no new warnings - [ ] I have added tests that prove my fix is effective or that my feature works - [ ] New and existing unit tests pass locally with my changes - [x] PR is being merged under a feature flag #### QA activity: - [ ] [Speedbreak features](https://github.com/appsmithorg/TestSmith/wiki/Guidelines-for-test-plans#speedbreakers-) have been covered - [ ] Test plan covers all impacted features and [areas of interest](https://github.com/appsmithorg/TestSmith/wiki/Guidelines-for-test-plans#areas-of-interest-) - [ ] Test plan has been peer reviewed by project stakeholders and other QA members - [ ] Manually tested functionality on DP - [ ] We had an implementation alignment call with stakeholders post QA Round 2 - [ ] Cypress test cases have been added and approved by SDET/manual QA - [ ] Added `Test Plan Approved` label after Cypress tests were reviewed - [ ] Added `Test Plan Approved` label after JUnit tests were reviewed <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit - **Refactor** - Updated the action button to have a conditional height based on the number of pages for improved layout consistency. <!-- end of auto-generated comment: release notes by coderabbit.ai -->
2 participants
Description
This PR adds a fixed height to the new query button in datasource config page.
PR fixes following issue(s)
Fixes #29609
Type of change
Testing
How Has This Been Tested?
Test Plan
Issues raised during DP testing
Checklist:
Dev activity
QA activity:
Test Plan Approvedlabel after Cypress tests were reviewedTest Plan Approvedlabel after JUnit tests were reviewedSummary by CodeRabbit