fix: allow currentRow for onClick in menuButton item in Table widget #35381
Conversation
SaiCharanChetpelly31
requested review from
ApekshaBhosale and
sagar-qa007
as code owners
WalkthroughThe recent changes enhance the functionality of a table widget by enabling the use of the Changes
Assessment against linked issues
Possibly related PRs
Suggested labels
Suggested reviewers
Poem
📜 Recent review detailsConfiguration used: .coderabbit.yaml 📒 Files selected for processing (1)
🧰 Additional context used📓 Path-based instructions (1)
🔇 Additional comments (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
|
![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, codebase verification and nitpick comments (9)
app/client/cypress/e2e/Regression/ClientSide/Widgets/TableV2/columnTypes/menubutton_spec.js (9)
Line range hint
14-14: Avoid usingcy.waitin tests.Using
cy.waitcan make tests flaky. Instead, use assertions to wait for elements to be in the desired state.- cy.wait(2000);
Line range hint
14-14: Use locator variables for selectors.Avoid using plain strings for selectors. Use locator variables instead.
- cy.get(".t--add-menu-item-btn").click(); + cy.get(_.locators._addMenuItemBtn).click();
Line range hint
14-14: Avoid usingcy.waitin tests.Using
cy.waitcan make tests flaky. Instead, use assertions to wait for elements to be in the desired state.- cy.wait(2000);
Line range hint
14-14: Use locator variables for selectors.Avoid using plain strings for selectors. Use locator variables instead.
- cy.get(".t--property-control-textcolor .t--js-toggle").click(); + cy.get(_.locators._propertyControlTextColorJsToggle).click();
Line range hint
14-14: Avoid usingcy.waitin tests.Using
cy.waitcan make tests flaky. Instead, use assertions to wait for elements to be in the desired state.- cy.wait(2000);
Line range hint
14-14: Use locator variables for selectors.Avoid using plain strings for selectors. Use locator variables instead.
- cy.get(".t--property-control-disabled .t--js-toggle").click(); + cy.get(_.locators._propertyControlDisabledJsToggle).click();
Line range hint
14-14: Avoid usingcy.waitin tests.Using
cy.waitcan make tests flaky. Instead, use assertions to wait for elements to be in the desired state.- cy.wait(2000);
Line range hint
14-14: Use locator variables for selectors.Avoid using plain strings for selectors. Use locator variables instead.
- cy.get(".t--property-control-visible .t--js-toggle").click(); + cy.get(_.locators._propertyControlVisibleJsToggle).click();
Line range hint
14-14: Use locator variables for selectors.Avoid using plain strings for selectors. Use locator variables instead.
- _.propPane.EnterJSContext("onClick", ""); - _.propPane.TypeTextIntoField("onClick", "{{currentR"); - _.agHelper.AssertElementExist(_.locators._hints); - _.agHelper.GetNAssertElementText( - _.locators._hints, - "currentRow", - "contain.text", - ); + _.propPane.EnterJSContext(_.locators._onClick, ""); + _.propPane.TypeTextIntoField(_.locators._onClick, "{{currentR"); + _.agHelper.AssertElementExist(_.locators._hints); + _.agHelper.GetNAssertElementText( + _.locators._hints, + "currentRow", + "contain.text", + );
Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Files selected for processing (2)
- app/client/cypress/e2e/Regression/ClientSide/Widgets/TableV2/columnTypes/menubutton_spec.js (1 hunks)
- app/client/src/widgets/TableWidgetV2/widget/propertyConfig/PanelConfig/Basic.ts (1 hunks)
Additional context used
Path-based instructions (1)
app/client/cypress/e2e/Regression/ClientSide/Widgets/TableV2/columnTypes/menubutton_spec.js (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 (1)
app/client/src/widgets/TableWidgetV2/widget/propertyConfig/PanelConfig/Basic.ts (1)
254-254: LGTM!The addition of the
customJSControlproperty enhances the flexibility and usability of the widget by allowing custom JavaScript control mechanisms.
|
@SaiCharanChetpelly31 Can you please explain me for the cypress test case requirement here? Can we test with Unit test ? |
@sagar-qa007 For other fields like You can check the already existing cypress tests for visible and disable fields here |
|
This PR has not seen activitiy for a while. It will be closed in 7 days unless further activity is detected. |
|
@sagar-qa007 @ApekshaBhosale I hope you're well. I’m curious and excited to hear your thoughts on my pull request raised a while ago. Looking forward to your review! |
|
This PR has not seen activitiy for a while. It will be closed in 7 days unless further activity is detected. |
|
@sagar-qa007 @ApekshaBhosale I hope you're well. I’m curious and excited to hear your thoughts on my pull request raised a while ago. Looking forward to your review! |
| @@ -163,5 +163,15 @@ describe( | |||
| }); | |||
| cy.get(".table-menu-button-popover li a").should("not.exist"); | |||
| }); | |||
| it("5. should check that menuitems onClick property has access to currentRow", () => { | |||
There was a problem hiding this comment.
Could you please help me understand the necessity of Cypress test cases in this context? I believe unit testing might be sufficient in this scenario.
Your insights would be appreciated.
There was a problem hiding this comment.
@sagar-qa007 I have mentioned the reason here.
#35381 (comment)
There was a problem hiding this comment.
I agree, this is an integration scenario(Table, property control and eval are involved), we need cypress tests.
|
This PR has not seen activitiy for a while. It will be closed in 7 days unless further activity is detected. |
|
@ApekshaBhosale Could you please review this PR? |
|
This PR has not seen activitiy for a while. It will be closed in 7 days unless further activity is detected. |
|
@SaiCharanChetpelly31 Please fix merge conflicts. |
sure, working on it |
SaiCharanChetpelly31
requested review from
sharat87,
ankitakinger,
ayushpahwa and
a team
as code owners
SaiCharanChetpelly31
force-pushed
the
fix_26052_tablewidget_menubutton
branch
from
e40b3c3
to
baadf6b
Compare
|
@rahulbarwal I resolved merge conflicts. can you please check? |
|
@jsartisan could you please review this PR? |
Fixes #26052
Summary by CodeRabbit
New Features
customJSControl, for enhanced configuration in the Table Widget, allowing for dynamic table data handling.onClickproperty correctly accesses thecurrentRowcontext, improving the robustness of the component.Bug Fixes