kie-issues#1547: DMN Editor: Render evaluation highlights in the Boxed Expression Editor #2681

jomarko · 2024-10-18T08:54:56Z

Closes: apache/incubator-kie-issues/issues/1547

checklist

decision table
conditional
context additional row
nested expressions

testing

New Evaluation Hits stories were added into Conditional and Decision Table stories.

… Editor Closes: apache/incubator-kie-issues/issues/1547

…tional

jomarko · 2024-10-30T12:26:31Z

packages/boxed-expression-component/src/table/BeeTable/BeeTable.css

@@ -203,6 +203,46 @@
  border: 0;
 }

+.expression-container .table-component tr.evaluation-highlights-row-overlay > td:first-child::before {


first-child here was my decision to use, otherwise there is problem, when then/ else branch has nested decision table, but not all decision table rules were evaluated. see:

ljmotta

@jomarko Thanks a lot for your PR. I didn't have time to manually test it, but I will. Until then, I've made some comments inline.

ljmotta · 2024-12-09T22:21:45Z

packages/boxed-expression-component/src/api/BeeTable.ts

+  /** 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;


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.

understand, done

ljmotta · 2024-12-09T22:22:56Z

...s/boxed-expression-component/src/expressions/ConditionalExpression/ConditionalExpression.tsx

@@ -247,6 +256,8 @@ export function ConditionalExpression({
          shouldRenderRowIndexColumn={false}
          shouldShowRowsInlineControls={false}
          shouldShowColumnsInlineControls={false}
+          evaluationHitsCountColumnIndex={1}


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

removed from the codebase

ljmotta · 2024-12-09T22:23:42Z

...xed-expression-component/src/expressions/DecisionTableExpression/DecisionTableExpression.tsx

@@ -1073,6 +1077,8 @@ export function DecisionTableExpression({
        shouldRenderRowIndexColumn={true}
        shouldShowRowsInlineControls={true}
        shouldShowColumnsInlineControls={true}
+        evaluationHitsCountColumnIndex={0}


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

removed from the codebase

packages/boxed-expression-component/src/BoxedExpressionEditorContext.tsx

ljmotta · 2024-12-09T22:34:28Z

packages/boxed-expression-component/src/table/BeeTable/BeeTableBody.tsx

+      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;


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.

the ternary operator is tricky

there is an eslint rule, that would force what you propose:

https://eslint.style/rules/js/multiline-ternary

However once I used it in project and formatted ternary operators as multiple lines, the current prettier configuration, the pre-commit hook for formatting we have reverted those changes, or adapted them in a way, that caused eslint again complaining..

So we have currently a prettier configuration that is not compatible with this eslint rule.

ljmotta · 2024-12-09T22:35:06Z

packages/boxed-expression-component/src/table/BeeTable/BeeTableTd.tsx

+  const evaluationHitsCountBadgeClassName = canDisplayEvaluationHitsCountBadge
+    ? (evaluationHitsCount ?? 0) > 0
+      ? "evaluation-hits-count-badge-colored"
+      : "evaluation-hits-count-badge-non-colored"
+    : "";


Please, use an useMemo hook here.

thank you, used

…ontext.tsx Co-authored-by: Luiz João Motta <[email protected]>

ljmotta · 2024-12-10T19:51:10Z

@jomarko Thanks for applying the changes.

After manually testing it, I could find a CSS that will highlight as first discussed:

.expression-container-box .conditional-expression .table-component tr.evaluation-hits-count-row-overlay > td:first-child::before {
  content: "";
  position: absolute;
  background-color: rgb(215, 201, 255, 0.5);
  left: 0;
  top: 0;
  width: 100%;
  height: 100%;
}

.expression-container-box .conditional-expression .table-component tr.evaluation-hits-count-row-overlay > td > div > div > div > .logic-type-selected-header::before {
  content: "";
  position: absolute;
  background-color: rgb(215, 201, 255, 0.5);
  left: 0;
  top: 0;
  width: 100%;
  height: 100%;
}

.expression-container-box .decision-table-expression .table-component tr.evaluation-hits-count-row-overlay > td::before {
  content: "";
  position: absolute;
  background-color: rgb(215, 201, 255, 0.5);
  left: 0;
  top: 0;
  width: 100%;
  height: 100%;
}

You will need to add a conditional-expression class to the ConditionalExpression.tsx file:

      <div className={"conditional-expression"} data-testid={"kie-tools--boxed-expression-component---conditional"}>
        <BeeTable<ROWTYPE>
           ...

I guess it can be simplified, but I broke into the conditional-expression, decision-table-expression and logic-type-selected-header CSS.

kbowers-ibm

Thanks @jomarko everything looks great to me!

ljmotta

@jomarko I've just one last comment regarding the boxedExpressionStoriesWrapper.

ljmotta · 2024-12-13T21:15:52Z

packages/boxed-expression-component/stories/boxedExpressionStoriesWrapper.tsx

It's not necessary to modify the BoxedExpressionEditorStoryArgs type, as the BoxedExpressionEditorProps already contains the evaluationHitsCountById property.

Also, the useState isn't required.

The evaluationHitsCountById should have the same value as other props:

evaluationHitsCountById={props?.evaluationHitsCountById ?? args?.evaluationHitsCountById}

thank you, I am happy we can keep it simple

jomarko added 3 commits October 18, 2024 10:49

kie-issues#1547: Render evaluation highlights in the Boxed Expression…

19bd400

… Editor Closes: apache/incubator-kie-issues/issues/1547

restrict ::before size

713e27d

Display evaluationHitsCount in the leading column of the DT and Condi…

d99ef6a

…tional

tiagobento changed the title ~~kie-issues#1547: Render evaluation highlights in the Boxed Expression…~~ kie-issues#1547: Render evaluation highlights in the Boxed Expression Editor Oct 22, 2024

tiagobento added the area:dmn label Oct 22, 2024

jomarko added 3 commits October 29, 2024 16:05

Merge branch 'main' into kie-issues#1547

262e4b5

Adapt props to Map<string, number>

f2274d0

Style the first column only

84a6a46

jomarko commented Oct 30, 2024

View reviewed changes

jomarko added 4 commits October 30, 2024 13:28

Prevent evaluationHitCount in 'if' row

527aad4

Merge branch 'main' into kie-issues#1547

925f925

Refactor variable names

22270b1

Cleanup

b2bf4a9

jomarko marked this pull request as ready for review October 30, 2024 15:11

jomarko requested a review from tiagobento as a code owner October 30, 2024 15:11

jomarko changed the title ~~kie-issues#1547: Render evaluation highlights in the Boxed Expression Editor~~ kie-issues#1547: DMN Editor: Render evaluation highlights in the Boxed Expression Editor Oct 31, 2024

ljmotta requested changes Dec 9, 2024

View reviewed changes

jomarko and others added 10 commits December 10, 2024 10:11

Merge branch 'main' into kie-issues#1547

d7b4635

Update packages/boxed-expression-component/src/BoxedExpressionEditorC…

22996a0

…ontext.tsx Co-authored-by: Luiz João Motta <[email protected]>

evaluationHitsCountPerId -> evaluationHitsCountById

0ec93a0

useMemo

6e4ea9b

supportsEvaluationHitsCount

19e8b23

inline variable

d52e553

missing hook dependencies

4312521

fix useMemo

d5194b2

split ternary operator to multiline

3991237

remove one ternary operator

4ca0587

jomarko added 2 commits December 11, 2024 14:54

fix styling: highlight whole row

080f4cd

add stories

1fe7f31

stories wrapper

156db3e

jomarko added the pr: wip PR is still under development label Dec 12, 2024

jomarko added 2 commits December 13, 2024 10:05

Merge branch 'main' into kie-issues#1547

8b20094

Remove evaluationHitsCountColumnIndex

f1f356e

jomarko removed the pr: wip PR is still under development label Dec 13, 2024

jomarko requested a review from kbowers-ibm December 13, 2024 13:06

kbowers-ibm approved these changes Dec 13, 2024

View reviewed changes

ljmotta requested changes Dec 13, 2024

View reviewed changes

Fix boxedExpressionStoriesWrapper.tsx

7ce7cdc

jomarko requested a review from ljmotta December 18, 2024 07:44

ljmotta approved these changes Dec 18, 2024

View reviewed changes

jomarko merged commit 74659fd into apache:main Dec 18, 2024
15 checks passed

jomarko deleted the kie-issues#1547 branch December 18, 2024 14:20

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

kie-issues#1547: DMN Editor: Render evaluation highlights in the Boxed Expression Editor #2681

kie-issues#1547: DMN Editor: Render evaluation highlights in the Boxed Expression Editor #2681

jomarko commented Oct 18, 2024 •

edited

Loading

jomarko Oct 30, 2024

ljmotta left a comment

ljmotta Dec 9, 2024

jomarko Dec 10, 2024

ljmotta Dec 9, 2024

jomarko Dec 13, 2024

ljmotta Dec 9, 2024

jomarko Dec 13, 2024

ljmotta Dec 9, 2024

jomarko Dec 10, 2024

ljmotta Dec 9, 2024

jomarko Dec 10, 2024

ljmotta commented Dec 10, 2024 •

edited

Loading

kbowers-ibm left a comment

ljmotta left a comment

ljmotta Dec 13, 2024

jomarko Dec 16, 2024

kie-issues#1547: DMN Editor: Render evaluation highlights in the Boxed Expression Editor #2681

kie-issues#1547: DMN Editor: Render evaluation highlights in the Boxed Expression Editor #2681

Conversation

jomarko commented Oct 18, 2024 • edited Loading

checklist

testing

Choose a reason for hiding this comment

ljmotta left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ljmotta commented Dec 10, 2024 • edited Loading

kbowers-ibm left a comment

Choose a reason for hiding this comment

ljmotta left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jomarko commented Oct 18, 2024 •

edited

Loading

ljmotta commented Dec 10, 2024 •

edited

Loading