chore: external merge request from Contributor #36558
Conversation
…menubutton' into external-contri/fix_26052_tablewidget_menubutton
rahulbarwal
requested review from
ApekshaBhosale and
sagar-qa007
as code owners
|
Important Review skippedDraft detected. Please check the settings in the CodeRabbit UI or the You can disable this status message by setting the WalkthroughThe pull request introduces enhancements to the Table Widget by adding a new property, Changes
Assessment against linked issues
Possibly related PRs
Suggested labels
Suggested reviewers
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
|
github-actions
bot
added
the
skip-changelog
![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/cypress/e2e/Regression/ClientSide/Widgets/TableV2/columnTypes/ColumnTypeMenubutton_spec.js (1)
166-175: Well done on adding a new test case, but let's make it even better!Class, I'm pleased to see you've added a new test case to check the
onClickproperty's access tocurrentRow. Your test follows many of our best practices, which is commendable. However, there's always room for improvement, so let's review a few points:
Good job using locator variables instead of plain strings. This makes our tests more maintainable.
I appreciate that you've avoided using
cy.wait,cy.pause, oragHelper.sleep(). This helps keep our tests efficient.To make our test even more robust, consider using data-* attributes for selectors. For example:
cy.get("[data-cy=onClick-field]").type("{{currentR");Let's make our assertion more specific. Instead of just checking if the hint contains "currentRow", we could assert the exact text or structure of the hint:
_.agHelper.GetNAssertElementText( _.locators._hints, "currentRow: { /* Properties of the current row */ }", "contain.text", );Remember, class, specific assertions help us catch unexpected changes more easily.
Keep up the good work, and don't forget to raise your hand if you have any questions!
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (2)
- app/client/cypress/e2e/Regression/ClientSide/Widgets/TableV2/columnTypes/ColumnTypeMenubutton_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/ColumnTypeMenubutton_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 (2)
app/client/src/widgets/TableWidgetV2/widget/propertyConfig/PanelConfig/Basic.ts (2)
254-254: Class, let's examine this new line of code.The addition of
customJSControl: "TABLE_COMPUTE_VALUE"to theonClickproperty is a significant improvement. This change allows us to use table-specific computations in the onClick event of menu items.Can anyone tell me how this relates to our lesson objective? That's right! It enables the use of
currentRowcontext in the onClick event, just as we aimed for in our project plan (issue #35381).Remember, students, this enhancement will make our Table widget more dynamic and user-friendly. It's a perfect example of how small changes can have a big impact on functionality.
Line range hint
1-454: Class, let's summarize our code review lesson.We've examined the changes made to the
Basic.tsfile, and I'm pleased to report that the modification aligns perfectly with our lesson objective. The addition of thecustomJSControlproperty to theonClickevent of menu items is a well-implemented solution that addresses the need forcurrentRowcontext in the Table widget's menu button items.This change enhances the widget's functionality without disrupting the existing codebase. It's a textbook example of clean, focused code improvement. Well done to the contributor!
Are there any questions about how this change impacts our Table widget's capabilities?
rahulbarwal
added
Widgets & Accelerators Pod
…/SaiCharanChetpelly31/appsmith into external-contri/fix_26052_tablewidget_menubutton
…o external-contri/fix_26052_tablewidget_menubutton
…o external-contri/fix_26052_tablewidget_menubutton
…o external-contri/fix_26052_tablewidget_menubutton
|
This PR has not seen activitiy for a while. It will be closed in 7 days unless further activity is detected. |
|
This PR has been closed because of inactivity. |
Description
Original PR: #35381
EE PR: https://github.com/appsmithorg/appsmith-ee/pull/5230
Fixes #35381
Warning
If no issue exists, please create an issue first, and check with the maintainers if the issue is valid.
Automation
/ok-to-test tags="@tag.All"
🔍 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
customJSControl, to enhance configuration options within the Table Widget.onClickfunctionality has access to thecurrentRowcontext.Tip
🟢 🟢 🟢 All cypress tests have passed! 🎉 🎉 🎉
Workflow run: https://github.com/appsmithorg/appsmith/actions/runs/11176452762
Commit: 7ce4760
Cypress dashboard.
Tags:
@tag.AllSpec:
Fri, 04 Oct 2024 12:51:36 UTC