Re: [PR] kie-issues#1547: DMN Editor: Render evaluation highlights in the Boxed Expression Editor [incubator-kie-tools]

["ljmotta (via GitHub)" <gi...@apache.org>]

ljmotta commented on code in PR #2681:
URL: https://github.com/apache/incubator-kie-tools/pull/2681#discussion_r1876847443


##########
packages/boxed-expression-component/src/expressions/DecisionTableExpression/DecisionTableExpression.tsx:
##########
@@ -1073,6 +1077,8 @@ export function DecisionTableExpression({
         shouldRenderRowIndexColumn={true}
         shouldShowRowsInlineControls={true}
         shouldShowColumnsInlineControls={true}
+        evaluationHitsCountColumnIndex={0}

Review Comment:
   I get the `0` here, but it would be good to add a comment too.



##########
packages/boxed-expression-component/src/expressions/ConditionalExpression/ConditionalExpression.tsx:
##########
@@ -247,6 +256,8 @@ export function ConditionalExpression({
           shouldRenderRowIndexColumn={false}
           shouldShowRowsInlineControls={false}
           shouldShowColumnsInlineControls={false}
+          evaluationHitsCountColumnIndex={1}

Review Comment:
   Why `1`? Could you please add a comment here? Or make it a variable with a very good name.



##########
packages/boxed-expression-component/src/api/BeeTable.ts:
##########
@@ -97,6 +97,10 @@ export interface BeeTableProps<R extends object> {
   shouldShowColumnsInlineControls: boolean;
   resizerStopBehavior: ResizerStopBehavior;
   lastColumnMinWidth?: number;
+  /** Index of the column, where the evaluation hits count should be displayed. If not set, evaluation hits count number is not shown. */
+  evaluationHitsCountColumnIndex?: number;
+  /** Method should return true for table rows, that can display evaluation hits count, false otherwise. If not set, BeeTableBody defaults to false. */
+  getEvaluationHitsCountSupportedByRow?: (row: ReactTable.Row<R>) => boolean;

Review Comment:
   The name is a little strange for me. The idea is to the get if a row supports (or not) the "EvaluationHitsCount". I don't think the "ByRow" makes sense, so:
   - `isEvaluationHitCountSupported(row: Row)`: The `is` tells the method will be boolean
   - `supportsEvaluationHitCount(row: Row)`
   
   Another idea is to use the `args: { row: Row }`, but I don't think is necessary, as we just have one parameter.



##########
packages/boxed-expression-component/src/BoxedExpressionEditorContext.tsx:
##########
@@ -34,6 +34,7 @@ export interface BoxedExpressionEditorContextType {
   pmmlDocuments?: PmmlDocument[];
   dataTypes: DmnDataType[];
   isReadOnly?: boolean;
+  evaluationHitsCountPerId?: Map<string, number>;

Review Comment:
   
   ```suggestion
     evaluationHitsCountById?: Map<string, number>;
   ```
   
   To keep consistency with other variables in the codebase.



##########
packages/boxed-expression-component/src/table/BeeTable/BeeTableTd.tsx:
##########
@@ -225,6 +231,12 @@ export function BeeTableTd<R extends object>({
     return onDataCellKeyUp?.(column.id);
   }, [column.id, onDataCellKeyUp]);
 
+  const evaluationHitsCountBadgeClassName = canDisplayEvaluationHitsCountBadge
+    ? (evaluationHitsCount ?? 0) > 0
+      ? "evaluation-hits-count-badge-colored"
+      : "evaluation-hits-count-badge-non-colored"
+    : "";

Review Comment:
   Please, use an `useMemo` hook here.



##########
packages/boxed-expression-component/src/table/BeeTable/BeeTableBody.tsx:
##########
@@ -72,15 +77,29 @@ export function BeeTableBody<R extends object>({
   lastColumnMinWidth,
   rowWrapper,
   isReadOnly,
+  evaluationHitsCountColumnIndex,
+  getEvaluationHitsCountSupportedByRow,
 }: BeeTableBodyProps<R>) {
+  const { evaluationHitsCountPerId } = useBoxedExpressionEditor();
+
   const renderRow = useCallback(
     (row: ReactTable.Row<R>, rowIndex: number) => {
       reactTableInstance.prepareRow(row);
 
+      const rowKey = getRowKey(row);
+      const isEvalationHitsCountSupportedByRow: boolean = getEvaluationHitsCountSupportedByRow?.(row) ?? false;
+      const rowEvaluationHitsCount = evaluationHitsCountPerId ? evaluationHitsCountPerId?.get(rowKey) ?? 0 : undefined;
+      const canDisplayEvaluationHitsCountRowOverlay =
+        rowEvaluationHitsCount !== undefined && isEvalationHitsCountSupportedByRow === true;
+      const rowClassName = canDisplayEvaluationHitsCountRowOverlay
+        ? rowKey + (rowEvaluationHitsCount > 0 ? " evaluation-hits-count-row-overlay" : "")
+        : rowKey;

Review Comment:
   This part is a little dense, a lot of information here.
    - Could you please inline the `isEvalationHitsCountSupportedByRow` as it's used only one time?
    - I'm not sure about this one, but can you force the ternary to be written in multiple lines?
    - Could you please add a line break between? As multiple ternaries can be very difficult to read.



-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: commits-unsubscribe@kie.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@kie.apache.org
For additional commands, e-mail: commits-help@kie.apache.org