fix: display label instead of value in select column of table widget #34224
Conversation
SaiCharanChetpelly31
requested review from
a team and
marks0351
and removed request for
a team
WalkthroughThe updates introduce enhanced functionality for the TableWidgetV2 in a client-side application with a new focus on select column display properties. Changes include incorporating a Changes
Poem
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 Configration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 6
Outside diff range and nitpick comments (45)
app/client/src/widgets/TableWidgetV2/widget/propertyConfig/PanelConfig/Select.ts (1)
Line range hint
94-96: Consider removing the else clause as it is unnecessary; the previous branches already cover all scenarios.- else { - return true; - }app/client/src/widgets/TableWidgetV2/component/cellComponents/SelectCell.tsx (1)
Line range hint
251-279: Consider removing the unnecessary else clause as it is redundant due to the early return in the previous block.- else { - const onEdit = () => toggleCellEditMode(true, rowIndex, alias, value); - return ( - <BasicCell - ... - ); - ); - }app/client/src/widgets/TableWidgetV2/component/Constants.ts (1)
186-186: Add documentation forselectDisplayAsproperty.Please add a comment explaining the purpose of the
selectDisplayAsproperty. This will enhance the maintainability of the code.app/client/src/widgets/TableWidgetV2/widget/propertyConfig/PanelConfig/Data.ts (1)
Line range hint
115-115: Change to an optional chain.Consider using an optional chain to handle potential undefined values more gracefully. This will improve the robustness of the code.
- const columnId = propertyPath.match(/primaryColumns\.(.*)\.alias/); + const columnId = propertyPath.match(/primaryColumns\.(.*)\.alias/)?.[1];app/client/src/widgets/TableWidgetV2/widget/utilities.ts (11)
Line range hint
130-132: Remove unnecessary else clause for cleaner and more efficient code.- } else { - return indices; - }
Line range hint
243-243: Consider using optional chaining for safer and cleaner code.- if (primaryColumns[columnId] && primaryColumns[columnId].isDerived) { + if (primaryColumns[columnId]?.isDerived) {Utilizing optional chaining here can prevent potential runtime errors if
primaryColumns[columnId]is undefined.
Line range hint
271-275: Remove unnecessary else clause to enhance code readability and efficiency.- } else { - return indices; - }
Line range hint
292-295: Remove redundant else clause to streamline the logic.- } else { - return indices; - }
Line range hint
296-298: Eliminate unnecessary else clause for improved code clarity.- } else { - return indices; - }
Line range hint
581-581: Address the issue of duplicate case labels to prevent logical errors.It appears there's a duplicate case label in the switch statement. This could lead to unexpected behavior as only the first matching case will execute. Please review and ensure each case is unique.
Line range hint
590-592: Omit the redundant else clause to simplify the code structure.- } else { - return indices; - }
Line range hint
874-876: Remove unnecessary else clause to streamline the logic flow.- } else { - return indices; - }
Line range hint
1118-1120: Eliminate the redundant else clause for a cleaner and more efficient codebase.- } else { - return indices; - }
Line range hint
1121-1123: Remove unnecessary else clause to simplify the code.- } else { - return indices; - }
Line range hint
1129-1133: Omit the redundant else clause to improve code readability and efficiency.- } else { - return indices; - }app/client/src/pages/Editor/PropertyPane/PropertyControl.tsx (10)
Line range hint
473-473: Consider using optional chaining to avoid potential runtime errors.- if (customJSControl && isDynamicValue(value)) { + if (customJSControl?.isDynamicValue(value)) {
Line range hint
523-523: Use optional chaining to simplify and enhance safety.- if (customJSControl && isDynamicValue(value)) { + if (customJSControl?.isDynamicValue(value)) {
Line range hint
564-564: Employ optional chaining for cleaner and safer code.- if (customJSControl && isDynamicValue(value)) { + if (customJSControl?.isDynamicValue(value)) {
Line range hint
642-642: ReplacehasOwnPropertywithObject.hasOwn()to avoid direct prototype method access.- if (editedName !== props.propertyName && widgetProperties.hasOwnProperty(editedName)) { + if (editedName !== props.propertyName && Object.hasOwn(widgetProperties, editedName)) {
Line range hint
649-681: Remove unnecessary else clause to simplify control flow.- } else if (editedName.trim() && editedName !== props.propertyName) { + if (editedName.trim() && editedName !== props.propertyName) {
Line range hint
709-709: Consider using optional chaining to simplify your conditional checks.- if (customJSControl && isDynamicValue(value)) { + if (customJSControl?.isDynamicValue(value)) {
Line range hint
711-712: Adopt optional chaining to improve readability and safety.- if (customJSControl && isDynamicValue(value)) { + if (customJSControl?.isDynamicValue(value)) {
Line range hint
762-762: Avoid using the delete operator. Consider setting the property to undefined instead.- onDeleteProperties([props.propertyName]); + widgetProperties[props.propertyName] = undefined;
Line range hint
899-899: Remove theautoFocusattribute to enhance accessibility and prevent unexpected focus behavior.- autoFocus
Line range hint
1006-1009: Explicitly set thetypeattribute of buttons to avoid unintended form submissions.+ type="button"app/client/src/widgets/TableWidgetV2/widget/propertyUtils.ts (1)
Line range hint
841-1069: Refactor the validation logic for select column options.The function
selectColumnOptionsValidationis quite complex and mixes different levels of abstraction. Consider breaking it down into smaller, more focused functions. For example, the validation of each option could be extracted into a separate function to improve readability and maintainability.app/client/cypress/support/Pages/AggregateHelper.ts (5)
Line range hint
879-879: The default parameter should ideally follow all required parameters or be converted to a required one. Please adjust the method signature accordingly.- public ScrollIntoView(selector: ElementType, index = 0, timeout = Cypress.config("defaultCommandTimeout")) { + public ScrollIntoView(selector: ElementType, timeout = Cypress.config("defaultCommandTimeout"), index = 0) {
Line range hint
1506-1507: Adjust the method parameters so that the default parameter follows all required parameters.- public AssertElementVisibility(selector: ElementType, visibility = true, index = 0, timeout = Cypress.config("pageLoadTimeout")) { + public AssertElementVisibility(selector: ElementType, index = 0, visibility = true, timeout = Cypress.config("pageLoadTimeout")) {
Line range hint
1536-1537: Simplify the ternary operation by directly using the boolean value.- return (visibility == true ? "be.visible" : "not.be.visible"); + return visibility ? "be.visible" : "not.be.visible";
Line range hint
1574-1578: The else clause here can be omitted as previous branches break early, simplifying the control flow.- else { - parentLoc.then(($parent) => { - if (!$parent.hasClass("t--switch-widget-inactive")) { - locator.click(); - } - }); - }
Line range hint
1590-1595: Another unnecessary else clause can be removed to clean up the code.- else { - locator = cy.get(propertyName); - locator.should("have.attr", "aria-checked", value); - }app/client/src/widgets/TableWidgetV2/widget/index.tsx (14)
Line range hint
275-277: Redundant double negation detected.- if (!!TableWidgetV2.getFeatureFlag(FEATURE_FLAG.rollout_js_enabled_one_click_binding_enabled)) + if (TableWidgetV2.getFeatureFlag(FEATURE_FLAG.rollout_js_enabled_one_click_binding_enabled))This change simplifies the condition by removing unnecessary double negation.
Line range hint
474-474: Incorrect use ofthisin a static context.- if (this.getFeatureFlag(ALLOW_TABLE_WIDGET_SERVER_SIDE_FILTERING)) + if (TableWidgetV2.getFeatureFlag(ALLOW_TABLE_WIDGET_SERVER_SIDE_FILTERING))Replace
thiswith the class nameTableWidgetV2in static methods to correctly reference static properties or methods.
Line range hint
651-651: Redundant double negation detected.- if (!!this.props.serverSidePaginationEnabled) + if (this.props.serverSidePaginationEnabled)This change simplifies the condition by removing unnecessary double negation.
Line range hint
694-696: Unnecessaryelseclause after early exit.- else { - //check if pageNo does not excede the max Page no, due to change of totalRecordsCount or change of pageSize - if (serverSidePaginationEnabled && totalRecordsCount) { - const maxAllowedPageNumber = Math.ceil(totalRecordsCount / pageSize); - if (pageNo > maxAllowedPageNumber) { - pushBatchMetaUpdates("pageNo", maxAllowedPageNumber); - this.updatePaginationDirectionFlags(PaginationDirection.NEXT_PAGE); - } - } - }The
elseblock can be removed to simplify the flow as the previousifblock contains a return statement.
Line range hint
723-723: Redundant double negation detected.- if (!!this.props.multiRowSelection) + if (this.props.multiRowSelection)This change simplifies the condition by removing unnecessary double negation.
Line range hint
789-789: Redundant double negation detected.- if (!!this.props.multiRowSelection) + if (this.props.multiRowSelection)This change simplifies the condition by removing unnecessary double negation.
Line range hint
1936-1936: Usage ofhasOwnPropertydirectly on object instances.- if (this.props.isEditableCellsValid?.hasOwnProperty(columnsAlias)) + if (Object.hasOwn(this.props.isEditableCellsValid, columnsAlias))Replace
hasOwnPropertywithObject.hasOwn()for safer property existence checks.Also applies to: 1937-1937
Line range hint
2151-2159: Declaration leakage inswitchstatement.+ { const onClick = column.onClick ? () => this.onColumnEvent({ rowIndex, action: column.onClick, triggerPropertyName: "onClick", eventType: EventType.ON_CLICK, }) : noop; + }Wrap declarations inside
switchcases in blocks to prevent leakage into other cases.
Line range hint
2182-2240: Declaration leakage inswitchstatement.+ { const getVisibleItems = (rowIndex: number) => { const { configureMenuItems, menuItems, menuItemsSource, sourceData } = cellProperties; ... }; + }Wrap declarations inside
switchcases in blocks to prevent leakage into other cases.
Line range hint
2194-2237: Unnecessaryelseclause after early exit.- else { - const visibleItems = sourceData - .map((item, index) => ({ - ...item, - id: index.toString(), - isVisible: getValue("isVisible", index, rowIndex), - isDisabled: getValue("isDisabled", index, rowIndex), - index: index, - widgetId: "", - label: getValue("label", index, rowIndex), - onClick: config?.onClick, - textColor: getValue("textColor", index, rowIndex), - backgroundColor: getValue("backgroundColor", index, rowIndex), - iconAlign: getValue("iconAlign", index, rowIndex), - iconColor: getValue("iconColor", index, rowIndex), - iconName: getValue("iconName", index, rowIndex), - })) - .filter((item) => item.isVisible === true); - }The
elseblock can be removed to simplify the flow as the previousifblock contains a return statement.
Line range hint
2211-2213: Unnecessaryelseclause after early exit.- else { - return []; - }The
elseblock can be removed to simplify the flow as the previousifblock contains a return statement.
Line range hint
2484-2484: Declaration leakage inswitchstatement.+ { const onClick = column.onClick ? () => this.onColumnEvent({ rowIndex, action: column.onClick, triggerPropertyName: "onClick", eventType: EventType.ON_CLICK, }) : noop; + }Wrap declarations inside
switchcases in blocks to prevent leakage into other cases.
Line range hint
2870-2870: Usage ofhasOwnPropertydirectly on object instances.- if (this.props.isEditableCellsValid.hasOwnProperty(columnsAlias)) + if (Object.hasOwn(this.props.isEditableCellsValid, columnsAlias))Replace
hasOwnPropertywithObject.hasOwn()for safer property existence checks.
Line range hint
2880-2882: Unnecessaryelseclause after early exit.- else { - pushBatchMetaUpdates("selectedRowIndices", []); - }The
elseblock can be removed to simplify the flow as the previousifblock contains a return statement.
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (15)
- app/client/cypress/e2e/Regression/ClientSide/Widgets/TableV2/columnTypes/Select3_spec.ts (1 hunks)
- app/client/cypress/support/Objects/CommonLocators.ts (1 hunks)
- app/client/cypress/support/Pages/AggregateHelper.ts (1 hunks)
- app/client/cypress/support/Pages/Table.ts (1 hunks)
- app/client/src/constants/PropertyControlConstants.tsx (1 hunks)
- app/client/src/pages/Editor/PropertyPane/PropertyControl.tsx (1 hunks)
- app/client/src/widgets/TableWidgetV2/component/Constants.ts (3 hunks)
- app/client/src/widgets/TableWidgetV2/component/cellComponents/SelectCell.tsx (7 hunks)
- app/client/src/widgets/TableWidgetV2/constants.ts (1 hunks)
- app/client/src/widgets/TableWidgetV2/widget/index.tsx (2 hunks)
- app/client/src/widgets/TableWidgetV2/widget/propertyConfig/PanelConfig/Data.ts (4 hunks)
- app/client/src/widgets/TableWidgetV2/widget/propertyConfig/PanelConfig/Select.ts (3 hunks)
- app/client/src/widgets/TableWidgetV2/widget/propertyUtils.test.ts (2 hunks)
- app/client/src/widgets/TableWidgetV2/widget/propertyUtils.ts (4 hunks)
- app/client/src/widgets/TableWidgetV2/widget/utilities.ts (1 hunks)
Files skipped from review due to trivial changes (2)
- app/client/cypress/support/Objects/CommonLocators.ts
- app/client/src/widgets/TableWidgetV2/constants.ts
Additional context used
Biome
app/client/src/widgets/TableWidgetV2/widget/propertyConfig/PanelConfig/Select.ts
[error] 94-96: This else clause can be omitted because previous branches break early. (lint/style/noUselessElse)
Unsafe fix: Omit the else clause.
app/client/cypress/e2e/Regression/ClientSide/Widgets/TableV2/columnTypes/Select3_spec.ts
[error] 108-116: This function expression can be turned into an arrow function. (lint/complexity/useArrowFunction)
Function expressions that don't use this can be turned into arrow functions.
Safe fix: Use an arrow function instead.
[error] 117-147: This function expression can be turned into an arrow function. (lint/complexity/useArrowFunction)
Function expressions that don't use this can be turned into arrow functions.
Safe fix: Use an arrow function instead.
[error] 148-186: This function expression can be turned into an arrow function. (lint/complexity/useArrowFunction)
Function expressions that don't use this can be turned into arrow functions.
Safe fix: Use an arrow function instead.app/client/src/widgets/TableWidgetV2/component/cellComponents/SelectCell.tsx
[error] 251-279: This else clause can be omitted because previous branches break early. (lint/style/noUselessElse)
Unsafe fix: Omit the else clause.
app/client/src/widgets/TableWidgetV2/widget/propertyConfig/PanelConfig/Data.ts
[error] 115-115: Change to an optional chain. (lint/complexity/useOptionalChain)
Unsafe fix: Change to an optional chain.
app/client/cypress/support/Pages/Table.ts
[error] 330-330: The assignment should not be in an expression. (lint/suspicious/noAssignInExpressions)
The use of assignments in expressions is confusing.
Expressions are often considered as side-effect free.
[error] 343-343: The assignment should not be in an expression. (lint/suspicious/noAssignInExpressions)
The use of assignments in expressions is confusing.
Expressions are often considered as side-effect free.
[error] 364-364: The assignment should not be in an expression. (lint/suspicious/noAssignInExpressions)
The use of assignments in expressions is confusing.
Expressions are often considered as side-effect free.
[error] 377-377: The assignment should not be in an expression. (lint/suspicious/noAssignInExpressions)
The use of assignments in expressions is confusing.
Expressions are often considered as side-effect free.
[error] 693-693: The assignment should not be in an expression. (lint/suspicious/noAssignInExpressions)
The use of assignments in expressions is confusing.
Expressions are often considered as side-effect free.
[error] 702-702: The assignment should not be in an expression. (lint/suspicious/noAssignInExpressions)
The use of assignments in expressions is confusing.
Expressions are often considered as side-effect free.
[error] 718-718: The assignment should not be in an expression. (lint/suspicious/noAssignInExpressions)
The use of assignments in expressions is confusing.
Expressions are often considered as side-effect free.app/client/src/widgets/TableWidgetV2/widget/utilities.ts
[error] 130-132: This else clause can be omitted because previous branches break early. (lint/style/noUselessElse)
Unsafe fix: Omit the else clause.
[error] 243-243: Change to an optional chain. (lint/complexity/useOptionalChain)
Unsafe fix: Change to an optional chain.
[error] 271-275: This else clause can be omitted because previous branches break early. (lint/style/noUselessElse)
Unsafe fix: Omit the else clause.
[error] 273-275: This else clause can be omitted because previous branches break early. (lint/style/noUselessElse)
[error] 292-295: This else clause can be omitted because previous branches break early. (lint/style/noUselessElse)
Unsafe fix: Omit the else clause.
[error] 296-298: This else clause can be omitted because previous branches break early. (lint/style/noUselessElse)
Unsafe fix: Omit the else clause.
[error] 581-581: Duplicate case label. (lint/suspicious/noDuplicateCase)
The first similar label is here:
[error] 590-592: This else clause can be omitted because previous branches break early. (lint/style/noUselessElse)
Unsafe fix: Omit the else clause.
[error] 874-876: This else clause can be omitted because previous branches break early. (lint/style/noUselessElse)
Unsafe fix: Omit the else clause.
[error] 1118-1120: This else clause can be omitted because previous branches break early. (lint/style/noUselessElse)
Unsafe fix: Omit the else clause.
[error] 1121-1123: This else clause can be omitted because previous branches break early. (lint/style/noUselessElse)
Unsafe fix: Omit the else clause.
[error] 1129-1133: This else clause can be omitted because previous branches break early. (lint/style/noUselessElse)
Unsafe fix: Omit the else clause.
[error] 1131-1133: This else clause can be omitted because previous branches break early. (lint/style/noUselessElse)
app/client/src/pages/Editor/PropertyPane/PropertyControl.tsx
[error] 473-473: Change to an optional chain. (lint/complexity/useOptionalChain)
Unsafe fix: Change to an optional chain.
[error] 523-523: Change to an optional chain. (lint/complexity/useOptionalChain)
Unsafe fix: Change to an optional chain.
[error] 564-564: Change to an optional chain. (lint/complexity/useOptionalChain)
Unsafe fix: Change to an optional chain.
[error] 642-642: Do not access Object.prototype method 'hasOwnProperty' from target object. (lint/suspicious/noPrototypeBuiltins)
It's recommended using Object.hasOwn() instead of using Object.hasOwnProperty().
See MDN web docs for more details.
[error] 649-681: This else clause can be omitted because previous branches break early. (lint/style/noUselessElse)
Unsafe fix: Omit the else clause.
[error] 709-709: Change to an optional chain. (lint/complexity/useOptionalChain)
Unsafe fix: Change to an optional chain.
[error] 711-712: Change to an optional chain. (lint/complexity/useOptionalChain)
Unsafe fix: Change to an optional chain.
[error] 762-762: Avoid the delete operator which can impact performance. (lint/performance/noDelete)
Unsafe fix: Use an undefined assignment instead.
[error] 899-899: Avoid the autoFocus attribute. (lint/a11y/noAutofocus)
Unsafe fix: Remove the autoFocus attribute.
[error] 1006-1009: Provide an explicit type prop for the button element. (lint/a11y/useButtonType)
The default type of a button is submit, which causes the submission of a form when placed inside a
formelement. This is likely not the behaviour that you want inside a React application.
Allowed button types are: submit, button or resetapp/client/src/widgets/TableWidgetV2/widget/propertyUtils.ts
[error] 1010-1010: expected
)but instead foundas(parse)Remove as
[error] 1010-1010: expected
,but instead foundArray(parse)Remove Array
[error] 1010-1010: expected
,but instead found)(parse)Remove )
[error] 1010-1010: Expected a statement but instead found '.'. (parse)
Expected a statement here.
[error] 1010-1010: Expected a semicolon or an implicit semicolon after a statement, but found none (parse)
An explicit or implicit semicolon is expected here...
...Which is required to end this statement
[error] 1015-1015: A
breakstatement can only be used within an enclosing iteration or switch statement. (parse)
[error] 1022-1022: A
breakstatement can only be used within an enclosing iteration or switch statement. (parse)
[error] 40-61: This else clause can be omitted because previous branches break early. (lint/style/noUselessElse)
Unsafe fix: Omit the else clause.
[error] 52-61: This else clause can be omitted because previous branches break early. (lint/style/noUselessElse)
[error] 80-86: This else clause can be omitted because previous branches break early. (lint/style/noUselessElse)
Unsafe fix: Omit the else clause.
[error] 105-117: This else clause can be omitted because previous branches break early. (lint/style/noUselessElse)
Unsafe fix: Omit the else clause.
[error] 111-117: This else clause can be omitted because previous branches break early. (lint/style/noUselessElse)
[error] 229-231: This else clause can be omitted because previous branches break early. (lint/style/noUselessElse)
Unsafe fix: Omit the else clause.
[error] 282-282: Change to an optional chain. (lint/complexity/useOptionalChain)
Unsafe fix: Change to an optional chain.
[error] 293-293: Change to an optional chain. (lint/complexity/useOptionalChain)
Unsafe fix: Change to an optional chain.
[error] 329-329: Change to an optional chain. (lint/complexity/useOptionalChain)
Unsafe fix: Change to an optional chain.
[error] 369-371: This else clause can be omitted because previous branches break early. (lint/style/noUselessElse)
Unsafe fix: Omit the else clause.
[error] 372-374: This else clause can be omitted because previous branches break early. (lint/style/noUselessElse)
Unsafe fix: Omit the else clause.
[error] 389-391: This else clause can be omitted because previous branches break early. (lint/style/noUselessElse)
Unsafe fix: Omit the else clause.
[error] 508-508: Change to an optional chain. (lint/complexity/useOptionalChain)
Unsafe fix: Change to an optional chain.
app/client/cypress/support/Pages/AggregateHelper.ts
[error] 851-854: This else clause can be omitted because previous branches break early. (lint/style/noUselessElse)
Unsafe fix: Omit the else clause.
[error] 879-879: This default parameter should follow the last required parameter or should be a required parameter. (lint/style/useDefaultParameterLast)
The last required parameter is here:
A default parameter that precedes a required parameter cannot be omitted at call site.
Unsafe fix: Turn the parameter into a required parameter.
[error] 1506-1507: This default parameter should follow the last required parameter or should be a required parameter. (lint/style/useDefaultParameterLast)
The last required parameter is here:
A default parameter that precedes a required parameter cannot be omitted at call site.
Unsafe fix: Turn the parameter into a required parameter.
[error] 1536-1537: Unnecessary use of boolean literals in conditional expression. (lint/complexity/noUselessTernary)
Simplify your code by directly assigning the result without using a ternary operator.
If your goal is negation, you may use the logical NOT (!) or double NOT (!!) operator for clearer and concise code.
Check for more details about NOT operator.
Unsafe fix: Remove the conditional expression with
[error] 1574-1578: This else clause can be omitted because previous branches break early. (lint/style/noUselessElse)
Unsafe fix: Omit the else clause.
[error] 1590-1595: This else clause can be omitted because previous branches break early. (lint/style/noUselessElse)
Unsafe fix: Omit the else clause.
app/client/src/widgets/TableWidgetV2/widget/index.tsx
[error] 275-277: Avoid redundant double-negation. (lint/complexity/noExtraBooleanCast)
It is not necessary to use double-negation when a value will already be coerced to a boolean.
Unsafe fix: Remove redundant double-negation
[error] 474-474: Using this in a static context can be confusing. (lint/complexity/noThisInStatic)
this refers to the class.
Unsafe fix: Use the class name instead.
[error] 651-651: Avoid redundant double-negation. (lint/complexity/noExtraBooleanCast)
It is not necessary to use double-negation when a value will already be coerced to a boolean.
Unsafe fix: Remove redundant double-negation
[error] 694-696: This else clause can be omitted because previous branches break early. (lint/style/noUselessElse)
Unsafe fix: Omit the else clause.
[error] 723-723: Avoid redundant double-negation. (lint/complexity/noExtraBooleanCast)
It is not necessary to use double-negation when a value will already be coerced to a boolean.
Unsafe fix: Remove redundant double-negation
[error] 789-789: Avoid redundant double-negation. (lint/complexity/noExtraBooleanCast)
It is not necessary to use double-negation when a value will already be coerced to a boolean.
Unsafe fix: Remove redundant double-negation
[error] 1936-1936: Do not access Object.prototype method 'hasOwnProperty' from target object. (lint/suspicious/noPrototypeBuiltins)
It's recommended using Object.hasOwn() instead of using Object.hasOwnProperty().
See MDN web docs for more details.
[error] 1937-1937: Do not access Object.prototype method 'hasOwnProperty' from target object. (lint/suspicious/noPrototypeBuiltins)
It's recommended using Object.hasOwn() instead of using Object.hasOwnProperty().
See MDN web docs for more details.
[error] 2151-2159: Other switch clauses can erroneously access this declaration.
Wrap the declaration in a block to restrict its access to the switch clause. (lint/correctness/noSwitchDeclarations)The declaration is defined in this switch clause:
Unsafe fix: Wrap the declaration in a block.
[error] 2182-2240: Other switch clauses can erroneously access this declaration.
Wrap the declaration in a block to restrict its access to the switch clause. (lint/correctness/noSwitchDeclarations)The declaration is defined in this switch clause:
Unsafe fix: Wrap the declaration in a block.
[error] 2194-2237: This else clause can be omitted because previous branches break early. (lint/style/noUselessElse)
Unsafe fix: Omit the else clause.
[error] 2211-2213: This else clause can be omitted because previous branches break early. (lint/style/noUselessElse)
Unsafe fix: Omit the else clause.
[error] 2484-2484: Other switch clauses can erroneously access this declaration.
Wrap the declaration in a block to restrict its access to the switch clause. (lint/correctness/noSwitchDeclarations)The declaration is defined in this switch clause:
Unsafe fix: Wrap the declaration in a block.
[error] 2870-2870: Do not access Object.prototype method 'hasOwnProperty' from target object. (lint/suspicious/noPrototypeBuiltins)
It's recommended using Object.hasOwn() instead of using Object.hasOwnProperty().
See MDN web docs for more details.
[error] 2880-2882: This else clause can be omitted because previous branches break early. (lint/style/noUselessElse)
Unsafe fix: Omit the else clause.
Additional comments not posted (14)
app/client/src/widgets/TableWidgetV2/widget/propertyConfig/PanelConfig/Select.ts (1)
101-125: The addition of theselectDisplayAsproperty with dropdown options for "Label" and "Value" is well-implemented and aligns with the PR's objective to allow users to choose between displaying labels or values. The default value being "label" ensures backward compatibility for existing tables which is thoughtful.app/client/src/constants/PropertyControlConstants.tsx (1)
58-60: The modification to allowhelperTextto accept an additionalpropertyNameparameter enhances flexibility and is correctly implemented. This change supports dynamic helper texts based on the property name, providing a more context-sensitive user experience in the property pane.app/client/src/widgets/TableWidgetV2/component/cellComponents/SelectCell.tsx (1)
Line range hint
104-245: The implementation of theselectDisplayAsproperty in theSelectCellcomponent is well-executed. The logic to determine the cell value based on the selected display option (label or value) is clear and should work as intended.app/client/src/widgets/TableWidgetV2/component/Constants.ts (1)
577-580: Enum declaration is well-defined.The
SelectOptionAccessorenum is correctly defined and will facilitate clear distinctions between label and value display options in select columns.app/client/src/widgets/TableWidgetV2/widget/propertyConfig/PanelConfig/Data.ts (2)
165-171: Dynamic helper text is well implemented.The dynamic helper text based on the
columnTypeis a good use of functional programming to provide context-sensitive help. This enhances the user experience by providing relevant information when needed.
93-93: Verify the update hook integration.Ensure that the
updateSelectColumnDisplayAsValuefunction is correctly integrated into the update hook array and check for any potential side effects on other properties.Verification successful
The
updateSelectColumnDisplayAsValuefunction is found in several files, includingpropertyUtils.ts,propertyUtils.test.ts, andData.ts. This indicates that the function is defined, tested, and referenced in the intended file. However, to ensure it is correctly integrated into the update hook array and to check for any potential side effects, further inspection is required.To proceed, I'll generate a script to specifically look for the integration of
updateSelectColumnDisplayAsValuewithin the update hook array and any associated side effects.
The
updateSelectColumnDisplayAsValuefunction is indeed integrated into the update hook array inPanelConfig/Data.tsas part of thecomposePropertyUpdateHookfunction. This confirms that the function is correctly included in the update hook mechanism.Here are the relevant locations:
app/client/src/widgets/TableWidgetV2/widget/propertyConfig/PanelConfig/Data.ts(lines indicating the integration into the update hook array)Given this context, the integration appears to be correctly implemented.
Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify that the updateSelectColumnDisplayAsValue function is accurately triggered by the update hook. # Test: Search for the function usage in the project. rg --type typescript $'updateSelectColumnDisplayAsValue'Length of output: 90
Script:
#!/bin/bash # Description: Verify that the updateSelectColumnDisplayAsValue function is accurately triggered by the update hook. # Test: Search for the function usage in the project. rg --type ts 'updateSelectColumnDisplayAsValue'Length of output: 933
Script:
#!/bin/bash # Description: Verify the integration of updateSelectColumnDisplayAsValue in the update hook array and check for side effects. # Test: Search for the update hook array and check its contents. rg --type ts 'updateHook' -A 10Length of output: 108895
app/client/src/widgets/TableWidgetV2/widget/propertyUtils.test.ts (2)
Line range hint
1-17: The imports and type definitions are correctly structured and appear to be in use. Good adherence to clean code principles by keeping imports organized and types clearly defined.
Line range hint
17-1020: The existing tests for other functionalities likeuniqueColumnNameValidation,updateColumnStyles, etc., remain unaffected by the new changes. It's important to ensure that these tests still pass after the modifications in the utility functions.app/client/src/widgets/TableWidgetV2/widget/utilities.ts (1)
520-524: Ensure consistent handling ofselectDisplayAsproperty.The addition of
selectDisplayAsproperty ingetCellPropertiesfunction aligns with the PR's objective to handle display options in select columns effectively. This change allows the property to be dynamically managed based on the row index, which is crucial for maintaining the correct display state across different rows.app/client/src/widgets/TableWidgetV2/widget/propertyUtils.ts (2)
777-791: Add defaultselectDisplayAswhen a new select column is created.The function
updateSelectColumnDisplayAsValuecorrectly checks if theselectDisplayAsproperty is not set when a new select column type is added and then sets it toLABELby default. This aligns with the PR's objective to display labels by default for new select columns.
1040-1069: Ensure computed values are checked against select options.The logic added to ensure that computed values are present in select options is crucial for data integrity. Please verify that this function is used correctly across the application to avoid runtime errors.
Verification successful
The
selectColumnOptionsValidationfunction is used extensively in various test files and configuration files within the codebase. This indicates that the function is well-tested and integrated into the application.
propertyUtils.test.tsfiles contain numerous test cases forselectColumnOptionsValidation.PanelConfig/Select.tsfiles include references toselectColumnOptionsValidation.This extensive usage and testing coverage suggest that the function's logic for ensuring computed values are checked against select options is being verified correctly across the application.
Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify that the computed values are correctly checked against the select options. # Test: Search for usage of `selectColumnOptionsValidation` function. rg --type python $'selectColumnOptionsValidation'Length of output: 79
Script:
#!/bin/bash # Description: Verify that the computed values are correctly checked against the select options. # Test: Search for usage of `selectColumnOptionsValidation` function in JavaScript/TypeScript files. rg --type js,ts 'selectColumnOptionsValidation'Length of output: 77
Script:
#!/bin/bash # Description: Verify that the computed values are correctly checked against the select options. # Test: Search for usage of `selectColumnOptionsValidation` function in JavaScript/TypeScript files. rg --type js --type ts 'selectColumnOptionsValidation'Length of output: 7405
app/client/cypress/support/Pages/AggregateHelper.ts (2)
612-615: The implementation of theSelectDropdownListmethod looks correct and aligns with the intended functionality.
Line range hint
851-854: Consider removing the unnecessary else clause as suggested by the linting tool to simplify the control flow.
[REFACTOR_SUGGESTion]- else { - $element[0].scrollIntoView(); - return $element; - }app/client/src/widgets/TableWidgetV2/widget/index.tsx (1)
2134-2136: Handling ofselectDisplayAsproperty inSelectCell.The implementation correctly handles the
selectDisplayAsproperty, using a fallback toSelectOptionAccessor.VALUEif not explicitly set. This ensures robustness in cases where the property might not be set.
| public ChangeColumnTypeWithoutNavigatingBackToPropertyPane( | ||
| columnName: string, | ||
| newDataType: columnTypeValues, | ||
| tableVersion: "v1" | "v2" = "v1", | ||
| ) { | ||
| this.EditColumn(columnName, tableVersion); | ||
| this.propPane.SelectPropertiesDropDown("Column type", newDataType); | ||
| this.assertHelper.AssertNetworkStatus("@updateLayout"); | ||
| } |
There was a problem hiding this comment.
Ensure Proper Error Handling for Network Requests
The ChangeColumnTypeWithoutNavigatingBackToPropertyPane method modifies column types and asserts network status but does not handle potential errors from the network request. This could lead to unhandled promise rejections if the network request fails.
Consider adding error handling for the network request to improve the robustness of this method. Here's a suggested modification:
557 public ChangeColumnTypeWithoutNavigatingBackToPropertyPane(
558 columnName: string,
559 newDataType: columnTypeValues,
560 tableVersion: "v1" | "v2" = "v1",
561 ) {
562 this.EditColumn(columnName, tableVersion);
563 this.propPane.SelectPropertiesDropDown("Column type", newDataType);
564- this.assertHelper.AssertNetworkStatus("@updateLayout");
564+ this.assertHelper.AssertNetworkStatus("@updateLayout").catch((error) => {
564+ console.error("Network request failed", error);
564+ });
565 }This modification uses .catch() to handle any errors that might occur during the network request, logging them to the console for easier debugging.
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. Thoroughly test & benchmark the code to ensure it meets the requirements.
| public ChangeColumnTypeWithoutNavigatingBackToPropertyPane( | |
| columnName: string, | |
| newDataType: columnTypeValues, | |
| tableVersion: "v1" | "v2" = "v1", | |
| ) { | |
| this.EditColumn(columnName, tableVersion); | |
| this.propPane.SelectPropertiesDropDown("Column type", newDataType); | |
| this.assertHelper.AssertNetworkStatus("@updateLayout"); | |
| } | |
| public ChangeColumnTypeWithoutNavigatingBackToPropertyPane( | |
| columnName: string, | |
| newDataType: columnTypeValues, | |
| tableVersion: "v1" | "v2" = "v1", | |
| ) { | |
| this.EditColumn(columnName, tableVersion); | |
| this.propPane.SelectPropertiesDropDown("Column type", newDataType); | |
| this.assertHelper.AssertNetworkStatus("@updateLayout").catch((error) => { | |
| console.error("Network request failed", error); | |
| }); | |
| } |
| _?: any, | ||
| ) { | ||
| let _isValid = true, | ||
| _parsed, | ||
| _message = ""; | ||
| _message: string | { name: string; message: string } = ""; |
There was a problem hiding this comment.
Clarify error message structure.
- _message: string | { name: string; message: string } = "";
+ _message: { name: string; message: string } | null = null;The type of _message should be consistent. It should either always be an object or null if no error is present. This change helps in maintaining a consistent structure for error handling.
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. Thoroughly test & benchmark the code to ensure it meets the requirements.
| _message: string | { name: string; message: string } = ""; | |
| _message: { name: string; message: string } | null = null; |
| @@ -385,6 +386,7 @@ export interface ColumnProperties | |||
| menuItemsSource?: MenuItemsSource; | |||
| configureMenuItems?: ConfigureMenuItems; | |||
| sourceData?: Array<Record<string, unknown>>; | |||
| selectDisplayAs?: SelectCellProperties["selectDisplayAs"]; | |||
There was a problem hiding this comment.
Ensure consistency in property type declaration.
The selectDisplayAs property should consistently use the same type declaration method as the rest of the properties in the ColumnProperties interface. Consider using a direct type instead of referencing SelectCellProperties["selectDisplayAs"].
- selectDisplayAs?: SelectCellProperties["selectDisplayAs"];
+ selectDisplayAs?: string;Committable suggestion was skipped due to low confidence.
| it("Check if property displayAs, helperText are visible", function () { | ||
| table.ChangeColumnType("gender", "Select", "v2"); | ||
| table.EditColumn("gender", "v2"); | ||
| agHelper.GetNAssertContains( | ||
| locators._helperText, | ||
| "Each computed value here represents default value/display value", | ||
| ); | ||
| agHelper.AssertElementExist(propPane._propertyControl("displayas")); | ||
| }); |
There was a problem hiding this comment.
Consider converting these function expressions to arrow functions to enhance readability and align with modern JavaScript practices.
- function () {
+ () => {Also applies to: 117-147, 148-186
Tools
Biome
[error] 108-116: This function expression can be turned into an arrow function. (lint/complexity/useArrowFunction)
Function expressions that don't use this can be turned into arrow functions.
Safe fix: Use an arrow function instead.
| describe("updateSelectColumnDisplayAsValue", () => { | ||
| it(`Should return an updates array when the column type is "select" and the selectDisplayAs value is not present.`, () => { | ||
| const tableProps = { | ||
| primaryColumns: { | ||
| gender: { | ||
| computedValue: ["male", "female"], | ||
| }, | ||
| columnType: ColumnTypesType.SELECT, | ||
| }, | ||
| }; | ||
|
|
||
| expect( | ||
| updateSelectColumnDisplayAsValue( | ||
| tableProps as unknown as TableWidgetProps, | ||
| "primaryColumns.gender.computedValue", | ||
| "select", | ||
| ), | ||
| ).toEqual([ | ||
| { | ||
| propertyPath: "primaryColumns.gender.selectDisplayAs", | ||
| propertyValue: "label", | ||
| }, | ||
| ]); | ||
| }); | ||
|
|
||
| it(`should return an undefined when column type is not "select" and selectDisplayAs value is present`, () => { | ||
| const tableProps = { | ||
| primaryColumns: { | ||
| gender: { | ||
| computedValue: ["male", "female"], | ||
| selectDisplayAs: "label", | ||
| }, | ||
| columnType: ColumnTypesType.TEXT, | ||
| }, | ||
| }; | ||
|
|
||
| expect( | ||
| updateSelectColumnDisplayAsValue( | ||
| tableProps as unknown as TableWidgetProps, | ||
| "primaryColumns.gender.computedValue", | ||
| "select", | ||
| ), | ||
| ).toEqual(undefined); | ||
| }); | ||
|
|
||
| it(`should return an undefined when column type is "select" and selectDisplayAs value is present`, () => { | ||
| const tableProps = { | ||
| primaryColumns: { | ||
| gender: { | ||
| computedValue: ["male", "female"], | ||
| selectDisplayAs: "label", | ||
| }, | ||
| columnType: ColumnTypesType.SELECT, | ||
| }, | ||
| }; | ||
|
|
||
| expect( | ||
| updateSelectColumnDisplayAsValue( | ||
| tableProps as unknown as TableWidgetProps, | ||
| "primaryColumns.gender.computedValue", | ||
| "select", | ||
| ), | ||
| ).toEqual(undefined); | ||
| }); |
There was a problem hiding this comment.
The tests for updateSelectColumnDisplayAsValue are comprehensive, covering different scenarios based on column types and the presence of the selectDisplayAs value. However, the test descriptions could be more descriptive to clarify the conditions under which each test case is expected to pass or fail.
Consider enhancing the descriptions for clarity. For example:
- it(`Should return an updates array when the column type is "select" and the selectDisplayAs value is not present.`, () => {
+ it(`Should update 'selectDisplayAs' to 'label' when the column type is 'select' and 'selectDisplayAs' is not explicitly set.`, () => {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. Thoroughly test & benchmark the code to ensure it meets the requirements.
| describe("updateSelectColumnDisplayAsValue", () => { | |
| it(`Should return an updates array when the column type is "select" and the selectDisplayAs value is not present.`, () => { | |
| const tableProps = { | |
| primaryColumns: { | |
| gender: { | |
| computedValue: ["male", "female"], | |
| }, | |
| columnType: ColumnTypesType.SELECT, | |
| }, | |
| }; | |
| expect( | |
| updateSelectColumnDisplayAsValue( | |
| tableProps as unknown as TableWidgetProps, | |
| "primaryColumns.gender.computedValue", | |
| "select", | |
| ), | |
| ).toEqual([ | |
| { | |
| propertyPath: "primaryColumns.gender.selectDisplayAs", | |
| propertyValue: "label", | |
| }, | |
| ]); | |
| }); | |
| it(`should return an undefined when column type is not "select" and selectDisplayAs value is present`, () => { | |
| const tableProps = { | |
| primaryColumns: { | |
| gender: { | |
| computedValue: ["male", "female"], | |
| selectDisplayAs: "label", | |
| }, | |
| columnType: ColumnTypesType.TEXT, | |
| }, | |
| }; | |
| expect( | |
| updateSelectColumnDisplayAsValue( | |
| tableProps as unknown as TableWidgetProps, | |
| "primaryColumns.gender.computedValue", | |
| "select", | |
| ), | |
| ).toEqual(undefined); | |
| }); | |
| it(`should return an undefined when column type is "select" and selectDisplayAs value is present`, () => { | |
| const tableProps = { | |
| primaryColumns: { | |
| gender: { | |
| computedValue: ["male", "female"], | |
| selectDisplayAs: "label", | |
| }, | |
| columnType: ColumnTypesType.SELECT, | |
| }, | |
| }; | |
| expect( | |
| updateSelectColumnDisplayAsValue( | |
| tableProps as unknown as TableWidgetProps, | |
| "primaryColumns.gender.computedValue", | |
| "select", | |
| ), | |
| ).toEqual(undefined); | |
| }); | |
| describe("updateSelectColumnDisplayAsValue", () => { | |
| it(`Should update 'selectDisplayAs' to 'label' when the column type is 'select' and 'selectDisplayAs' is not explicitly set.`, () => { | |
| const tableProps = { | |
| primaryColumns: { | |
| gender: { | |
| computedValue: ["male", "female"], | |
| }, | |
| columnType: ColumnTypesType.SELECT, | |
| }, | |
| }; | |
| expect( | |
| updateSelectColumnDisplayAsValue( | |
| tableProps as unknown as TableWidgetProps, | |
| "primaryColumns.gender.computedValue", | |
| "select", | |
| ), | |
| ).toEqual([ | |
| { | |
| propertyPath: "primaryColumns.gender.selectDisplayAs", | |
| propertyValue: "label", | |
| }, | |
| ]); | |
| }); | |
| it(`should return an undefined when column type is not "select" and selectDisplayAs value is present`, () => { | |
| const tableProps = { | |
| primaryColumns: { | |
| gender: { | |
| computedValue: ["male", "female"], | |
| selectDisplayAs: "label", | |
| }, | |
| columnType: ColumnTypesType.TEXT, | |
| }, | |
| }; | |
| expect( | |
| updateSelectColumnDisplayAsValue( | |
| tableProps as unknown as TableWidgetProps, | |
| "primaryColumns.gender.computedValue", | |
| "select", | |
| ), | |
| ).toEqual(undefined); | |
| }); | |
| it(`should return an undefined when column type is "select" and selectDisplayAs value is present`, () => { | |
| const tableProps = { | |
| primaryColumns: { | |
| gender: { | |
| computedValue: ["male", "female"], | |
| selectDisplayAs: "label", | |
| }, | |
| columnType: ColumnTypesType.SELECT, | |
| }, | |
| }; | |
| expect( | |
| updateSelectColumnDisplayAsValue( | |
| tableProps as unknown as TableWidgetProps, | |
| "primaryColumns.gender.computedValue", | |
| "select", | |
| ), | |
| ).toEqual(undefined); | |
| }); |
| @@ -41,6 +41,7 @@ import { | |||
| SORT_ORDER, | |||
| SortOrderTypes, | |||
| StickyType, | |||
| SelectOptionAccessor, | |||
There was a problem hiding this comment.
Use of SelectOptionAccessor enum without explicit import.
Please ensure that SelectOptionAccessor is properly imported from the appropriate module to avoid runtime errors.
|
HI @ApekshaBhosale @marks0351 can you please review this PR? |
marks0351
requested review from
riodeuno and
jsartisan
and removed request for
marks0351
|
Let me check this today. |
|
@SaiCharanChetpelly31 I tried the PR on my local and couldn't get it working. For some reason, the select options always shows failed to validate for me |
|
@jsartisan can you try giving options in this way? |
|
Hey @SaiCharanChetpelly31 I am closing the PR as it does not work as intended. Also please check our contribution guidelines. |
|
@jsartisan can you please try to give array of objects? |
|
I tried the same. Passed an array of objects. It still does not work. 😕 Anyway, I would suggest you to pick something from the good first issue. By the way, I am really impressed that you understood the code of the table widget without any help. Great job on it but really sorry, we won't be accepting table widget contribution PR for now. It really consumes a lot of time for us and we are tight on bandwidth right now 🥲 Really sorry for this. |
@jsartisan Sure i will pick good first issues. Thanks in advance. |
|
Tried that too |
Fixed issue 26188
This pull request adds a new feature called "Display as" for the select column type in the table widget.
Whenever a column is set to select, by default we show labels and not values.
For existing tables, since we have created this problem we ensure we don't break their apps. We will give an option called "Display As" as suggested in the original issue and for new select columns, it will be Label and for existing columns, it will be Value.
We make it clear that the computed value here refers to the default value/display value. Displaying this as the helper text below the computed value property only for select column type.
Added a helper text below the computed value field that indicates that the computed value here is a default value/display value
Validation:When computed value is not present in the select option then we throw an error in select option. This is irrespective of displayAs property.
-For new row select options, there is no validation of computed value. Since are adding new data into the table and it has to adhere to the select options that we provide.
For displaying value in cell,
When display as is set to label, we show empty values for missing labels in the widget
When display as is set to value, we show the computed value as it is even if labels are missing
When computed value is not present in the select options, we still make the select options available. We do this because its not the select options that has issue but its computed value.
Summary by CodeRabbit
New Features
selectDisplayAsproperty to display options as label or value.PropertyControlcomponent to support enhancedhelperTextfunctionality.Improvements
COMPUTED_VALUE_SELECT_HELPER_TEXTfor clearer display of computed values in the table widget.Bug Fixes
AggregateHelperclass.Tests
selectDisplayAsproperty and related functionalities in the table widget.