feat: add sortBy property to table select cell type #35187
Conversation
…o feat/display-label-in-table-select-column-cell
…s://github.com/appsmithorg/appsmith into feat/add-sort-by-property-to-table-select-cell-type
WalkthroughThe recent enhancements to the Table widget's Select column type vastly improve its functionality and user experience. Key updates include the implementation of feature flags for toggling between label and value displays, a refined structure for options, and enhanced sorting capabilities. Collectively, these modifications streamline user interactions, clarify data presentation, and empower users with greater control over how information is displayed and sorted in the widget. Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant TableWidget
participant SelectCell
participant FeatureFlag
User->>TableWidget: Interacts with select column
TableWidget->>SelectCell: Passes current selection
SelectCell->>FeatureFlag: Checks if label/value feature is enabled
FeatureFlag-->>SelectCell: Returns feature status
SelectCell->>SelectCell: Determines display value based on feature status
SelectCell-->>TableWidget: Returns display value
TableWidget-->>User: Shows updated select column
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 as PR comments)
Additionally, you can add CodeRabbit Configuration File (
|
github-actions
bot
added
Widgets Product
…o feat/add-sort-by-property-to-table-select-cell-type
![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
Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Files selected for processing (6)
- app/client/cypress/e2e/Regression/ClientSide/Widgets/TableV2/columnTypes/Select1_spec.ts (2 hunks)
- app/client/src/ce/entities/FeatureFlag.ts (2 hunks)
- app/client/src/widgets/TableWidgetV2/component/Constants.ts (3 hunks)
- app/client/src/widgets/TableWidgetV2/component/cellComponents/SelectCell.tsx (4 hunks)
- app/client/src/widgets/TableWidgetV2/widget/derived.js (3 hunks)
- app/client/src/widgets/TableWidgetV2/widget/propertyConfig/PanelConfig/Select.ts (2 hunks)
Additional context used
Path-based instructions (1)
app/client/cypress/e2e/Regression/ClientSide/Widgets/TableV2/columnTypes/Select1_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 (18)
app/client/src/ce/entities/FeatureFlag.ts (2)
37-38: Good job adding the new feature flag!The new feature flag
release_table_cell_label_value_enabledis correctly added to theFEATURE_FLAGconstant. This addition will allow for dynamic toggling of the feature. Ensure to follow the naming convention as per the guidelines.
68-68: Default value set correctly!The default value for the new feature flag
release_table_cell_label_value_enabledis set tofalsein theDEFAULT_FEATURE_FLAG_VALUEobject. This is a good practice to ensure the feature is disabled by default until explicitly enabled.app/client/src/widgets/TableWidgetV2/widget/propertyConfig/PanelConfig/Select.ts (2)
10-10: Good import!The import of
TableSelectColumnOptionKeysis necessary for the newsortByproperty configuration. This ensures the options for the dropdown are correctly referenced.
41-66: Excellent addition of thesortByproperty!The new
sortByproperty is well-defined with a clear label, help text, and default value. The options for sorting by "label" or "value" are correctly structured, and the validation ensures only allowed values are used.app/client/src/widgets/TableWidgetV2/component/cellComponents/SelectCell.tsx (6)
1-1: Good import!The import of
FEATURE_FLAGis necessary for the new feature flag mechanism. This ensures the feature flag is correctly referenced.
4-4: Nice use of theuseFeatureFlaghook!The import of
useFeatureFlagis essential for checking the status of the new feature flag. This allows the component to dynamically respond to feature toggles.
197-199: Correct usage of theuseFeatureFlaghook!The
useFeatureFlaghook is correctly used to check the status ofrelease_table_cell_label_value_enabled. This ensures the component can dynamically toggle the feature based on the flag's status.
201-212: Well-implemented logic forcellLabelValue!The
useMemohook is used effectively to compute thecellLabelValuebased on the feature flag. This ensures that the label is used when the feature is enabled, otherwise, the value is used.
252-252: Correct usage ofcellLabelValue!The
cellLabelValueis correctly used in theStyledSelectComponentto ensure the correct label or value is displayed based on the feature flag.
282-282: Correct usage ofcellLabelValue!The
cellLabelValueis also correctly used in theBasicCellcomponent to ensure consistency in the displayed label or value.app/client/cypress/e2e/Regression/ClientSide/Widgets/TableV2/columnTypes/Select1_spec.ts (2)
8-8: Good addition of the import statement.The new import for
featureFlagInterceptis necessary for the feature flagging functionality.
157-193: Well-structured test case update.The test case updates correctly reflect the new feature flag and label-based sorting functionality.
However, ensure that
featureFlagInterceptis used correctly throughout the codebase.Verification successful
Consistent usage of
featureFlagInterceptacross the codebase.The function
featureFlagInterceptis consistently used across various test files to enable different feature flags, ensuring the correct behavior of the tests.
app/client/cypress/support/Pages/AutoLayout.tsapp/client/cypress/support/Pages/AdminSettings.tsapp/client/cypress/support/commands.jsapp/client/cypress/e2e/Regression/ClientSide/PartialImportExport/PartialExport_spec.tsapp/client/cypress/e2e/Regression/ClientSide/Widgets/TableV2/columnTypes/Select1_spec.ts- And many more...
Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify the usage of `featureFlagIntercept` in the codebase. # Test: Search for the usage of `featureFlagIntercept`. Expect: Consistent usage. rg --type js --type ts 'featureFlagIntercept'Length of output: 11361
app/client/src/widgets/TableWidgetV2/component/Constants.ts (3)
186-186: Good addition of thesortByproperty.The new optional property
sortByenhances theSelectCellPropertiesinterface by allowing sorting options to be specified.
345-345: Good addition of thesortByproperty.The new optional property
sortByenhances theEditActionColumnPropertiesinterface by allowing sorting options to be specified.
577-580: Good addition of theTableSelectColumnOptionKeysenum.The new enum standardizes the keys used for table selection options, improving type safety and clarity.
app/client/src/widgets/TableWidgetV2/widget/derived.js (3)
320-334: Good identification logic for select columns.The code correctly identifies select columns that need to be sorted by label, which is essential for the new sorting functionality.
336-358: Good data transformation logic for sorting by label.The code correctly transforms the
processedTableDatato replace the values in select columns with their corresponding labels for sorting.
Line range hint
394-470:
Well-implemented conditional sorting logic and final data transformation.The code ensures that the data is sorted by label but retains the correct value representation, which is essential for the new sorting functionality.
jacquesikot
added
ok-to-test
There was a problem hiding this comment.
Actionable comments posted: 0
Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Files selected for processing (2)
- app/client/cypress/e2e/Regression/ClientSide/Widgets/TableV2/TableV2_Sorting_spec.js (2 hunks)
- app/client/cypress/locators/commonlocators.json (2 hunks)
Additional context used
Path-based instructions (2)
app/client/cypress/e2e/Regression/ClientSide/Widgets/TableV2/TableV2_Sorting_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.
app/client/cypress/locators/commonlocators.json (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 (7)
app/client/cypress/e2e/Regression/ClientSide/Widgets/TableV2/TableV2_Sorting_spec.js (6)
7-52: Good use of sample data for testing.The
demoTableDatastructure is well-defined and provides a comprehensive dataset for testing sorting functionality.
53-59: Great use ofbeforeEachfor test isolation.Using
beforeEachto add a new page before each test ensures test isolation and reliability.
Line range hint
62-117:
Comprehensive test case with multiple assertions.The test case thoroughly verifies sorting for a custom column, including renaming and preserving data. Ensure that the test case covers all edge cases.
119-157: Clear and focused test case for default sorting.The test case effectively verifies default sorting for a select column using the value property. Ensure that the feature flag is appropriately managed.
159-201: Well-structured test case for sorting by label.The test case accurately verifies sorting for a select column type when
sortByis set to label. Ensure that the feature flag is appropriately managed.
1-6: Ensure necessary imports.The new import statements seem appropriate. Verify that all imported modules are used in the code.
Verification successful
All imported modules are used in the code.
The verification confirms that the imported modules
featureFlagIntercept,ObjectsCore,testdata,commonlocators, andPageListare utilized within the fileapp/client/cypress/e2e/Regression/ClientSide/Widgets/TableV2/TableV2_Sorting_spec.js.
featureFlagInterceptis used for feature flagging.ObjectsCoreis used for core object functionalities.testdatais used for test data.commonlocatorsis used for common locators.PageListis used for page list functionalities.Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify usage of imported modules. # Test: Search for usage of imported modules. Expect: All imported modules are used. rg --type js 'featureFlagIntercept|ObjectsCore|testdata|commonlocators|PageList'Length of output: 246341
app/client/cypress/locators/commonlocators.json (1)
109-109: New locator added for sorting control.The new locator
changeSortByis appropriately named and follows the existing naming conventions. Ensure it is used correctly in the tests.
app/client/cypress/e2e/Regression/ClientSide/Widgets/TableV2/TableV2_Sorting_spec.js
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 0
Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Files selected for processing (1)
- app/client/src/widgets/TableWidgetV2/widget/derived.js (3 hunks)
Files skipped from review as they are similar to previous changes (1)
- app/client/src/widgets/TableWidgetV2/widget/derived.js
There was a problem hiding this comment.
Actionable comments posted: 0
Outside diff range, codebase verification and nitpick comments (4)
app/client/cypress/e2e/Regression/ClientSide/Widgets/TableV2/TableV2_Sorting_spec.js (4)
1-5: Use locator pages from CommonLocators.tsPlease use locator pages from
app/client/cypress/support/Objects/CommonLocators.tsas per previous review comments.
Line range hint
62-117:
Avoid using plain strings for locatorsPlease use locator variables instead of plain strings for locators. This improves maintainability and readability.
Good use of multiple assertions
The test case uses multiple assertions to verify sorting functionality. This is a good practice.
119-157: Avoid using cy.waitPlease avoid using
cy.waitin your test code. Instead, use more reliable methods likecy.interceptandcy.waitfor specific network requests.Use locator variables
Ensure that locator variables are used instead of plain strings for selectors.
159-201: Avoid using cy.waitPlease avoid using
cy.waitin your test code. Instead, use more reliable methods likecy.interceptandcy.waitfor specific network requests.Use locator variables
Ensure that locator variables are used instead of plain strings for selectors.
Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Files selected for processing (1)
- app/client/cypress/e2e/Regression/ClientSide/Widgets/TableV2/TableV2_Sorting_spec.js (3 hunks)
Additional context used
Path-based instructions (1)
app/client/cypress/e2e/Regression/ClientSide/Widgets/TableV2/TableV2_Sorting_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/cypress/e2e/Regression/ClientSide/Widgets/TableV2/TableV2_Sorting_spec.js (2)
7-52: LGTM!The
demoTableDatavariable is well-structured and suitable for testing the table sorting functionality.
58-59: Good use of beforeEach hookReplacing
beforewithbeforeEachenhances test isolation and reliability.
app/client/src/widgets/TableWidgetV2/widget/propertyConfig/PanelConfig/Select.ts
Outdated
Show resolved
Hide resolved
app/client/cypress/e2e/Regression/ClientSide/Widgets/TableV2/TableV2_Sorting_spec.js
Show resolved
Hide resolved
app/client/cypress/e2e/Regression/ClientSide/Widgets/TableV2/TableV2_Sorting_spec.js
Show resolved
Hide resolved
app/client/cypress/e2e/Regression/ClientSide/Widgets/TableV2/TableV2_Sorting_spec.js
Show resolved
Hide resolved
app/client/cypress/e2e/Regression/ClientSide/Widgets/TableV2/TableV2_Sorting_spec.js
Show resolved
Hide resolved
jacquesikot
added
ok-to-test
There was a problem hiding this comment.
Actionable comments posted: 0
Outside diff range, codebase verification and nitpick comments (4)
app/client/cypress/e2e/Regression/ClientSide/Widgets/TableV2/TableV2_Sorting_spec.js (4)
1-6: Great! But consider using locator pages for imports.The imports look good, but consider using locator pages from
app/client/cypress/support/Objects/CommonLocators.tsfor consistency.-const commonlocators = require("../../../../../locators/commonlocators.json"); +import commonlocators from "../../../../../support/Objects/CommonLocators";
Line range hint
62-117:
Excellent test case! But ensure the use of locator variables.The test case is well-written and covers the necessary scenarios for sorting and renaming a custom column. However, ensure that locator variables are used instead of plain strings.
- cy.get(_.propPane._paneTitle).click({ force: true }); - cy.get(_.propPane._paneTitle) + cy.get(commonlocators.paneTitle).click({ force: true }); + cy.get(commonlocators.paneTitle)
119-169: Good test case! But use locator variables and avoidcy.wait.The test case is well-written and covers the necessary scenarios for default sorting. However, ensure that locator variables are used and avoid using
cy.wait.- cy.get(commonlocators.changeColType).last().click(); - cy.get(_.locators._dropdownText).children().contains("Select").click(); - cy.wait("@updateLayout"); + cy.get(commonlocators.changeColType).last().click(); + cy.get(commonlocators.dropdownText).children().contains("Select").click();
171-225: Great test case! But use locator variables and avoidcy.wait.The test case is well-written and covers the necessary scenarios for sorting by label. However, ensure that locator variables are used and avoid using
cy.wait.- cy.get(commonlocators.changeSortBy).last().click(); - cy.get(_.locators._dropdownText).children().contains("Label").click(); - cy.wait("@updateLayout"); + cy.get(commonlocators.changeSortBy).last().click(); + cy.get(commonlocators.dropdownText).children().contains("Label").click();
Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Files selected for processing (3)
- app/client/cypress/e2e/Regression/ClientSide/Widgets/TableV2/TableV2_Sorting_spec.js (3 hunks)
- app/client/src/widgets/TableWidgetV2/widget/derived.js (3 hunks)
- app/client/src/widgets/TableWidgetV2/widget/propertyConfig/PanelConfig/Select.ts (2 hunks)
Files skipped from review as they are similar to previous changes (2)
- app/client/src/widgets/TableWidgetV2/widget/derived.js
- app/client/src/widgets/TableWidgetV2/widget/propertyConfig/PanelConfig/Select.ts
Additional context used
Path-based instructions (1)
app/client/cypress/e2e/Regression/ClientSide/Widgets/TableV2/TableV2_Sorting_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/cypress/e2e/Regression/ClientSide/Widgets/TableV2/TableV2_Sorting_spec.js (2)
7-52: Well done! The test data is well-structured.The
demoTableDatais well-structured and provides a comprehensive dataset for testing.
58-59: Good use ofbeforeEachfor test isolation.Using
beforeEachensures that each test starts with a clean state, which is a best practice in Cypress testing.
There was a problem hiding this comment.
Actionable comments posted: 0
Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Files selected for processing (1)
- app/client/src/widgets/TableWidgetV2/widget/derived.js (3 hunks)
Files skipped from review as they are similar to previous changes (1)
- app/client/src/widgets/TableWidgetV2/widget/derived.js
jacquesikot
added
ok-to-test
There was a problem hiding this comment.
Actionable comments posted: 0
Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Files selected for processing (1)
- app/client/src/widgets/TableWidgetV2/widget/derived.js (3 hunks)
Files skipped from review as they are similar to previous changes (1)
- app/client/src/widgets/TableWidgetV2/widget/derived.js
| const isColumnSortedByLabel = | ||
| column?.columnType === "select" && | ||
| column?.sortBy === "label" && | ||
| column?.selectOptions?.length; |
There was a problem hiding this comment.
Is it possible to implement the same logic without a specific check? I am wondering why we required such a check only for this case.
There was a problem hiding this comment.
I don’t think so. The sort by label logic I implemented should only take effect if those checks are true for specific fields, if those checks do not exist, we need to sort by value, which is the default method for sorting.
Nikhil-Nandagopal
removed
Widgets & Blocks Pod
Widgets Product
github-actions
bot
added
the
Widgets Product
jacquesikot
deleted the
feat/add-sort-by-property-to-table-select-cell-type
branch
|
/build-deploy-preview skip-tests=true |
|
Deploying Your Preview: https://github.com/appsmithorg/appsmith/actions/runs/10244885852. |
Description
Problem
The table widget now supports the Select column type, which allows the column to contain both a label and a value. This could be useful for currency fields, foreign keys, or any other case where you want to display a different version of the column value. However, there is a problem with sorting. The table always sorts using the value, and does not give the user an option to sort using the label, where it makes sense in specific cases.
Solution
This PR adds a Sort By property to the table select cell, allowing the users to choose which value is used for sorting without affecting any functionality of the label or value of the select cell.
Additional Technical Documentation
Tested Cases
Objective: Ensure that a newly added table with select column default sorts by the value
Steps:
Objective: Ensure that a newly added table with select column and sortBy property set to label sorts correctly
Steps:
Objective: Ensure that every other cell type in the table sorts correctly
Steps:
Objective: Ensure that the sorting works for all columns while using data binding in table data
Steps:
Objective: Ensure that the sorting works for all columns while using data binding in table data with a select column set to sort by the label
Steps:
Steps:
Steps:
Fixes #21993
Automation
/ok-to-test tags="@tag.Table, @tag.Binding, @tag.Sanity"
🔍 Cypress test results
Tip
🟢 🟢 🟢 All cypress tests have passed! 🎉 🎉 🎉
Workflow run: https://github.com/appsmithorg/appsmith/actions/runs/10165119164
Commit: 8a4e8b2
Cypress dashboard.
Tags:
@tag.Table, @tag.Binding, @tag.SanitySpec:
Tue, 30 Jul 2024 16:02:35 UTC
Communication
Should the DevRel and Marketing teams inform users about this change?
Summary by CodeRabbit
New Features
Bug Fixes
Tests
Documentation