Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

kie-issues#1741: Sandbox: Add “Open Boxed Expression Editor” button in the DMN Runner table output columns #2849

Draft
wants to merge 44 commits into
base: main
Choose a base branch
from

Conversation

jomarko
Copy link
Contributor

@jomarko jomarko commented Jan 10, 2025

Closes apache/incubator-kie-issues#1741

The dmn runner outputs table introduces a button in the header cell top right corner for navigating into the bound expression. It means the boxed expression editor is open for the given dmn runner result.

Screenshot 2025-01-27 122459

@jomarko jomarko added pr: DO NOT MERGE Draft PR, not ready for merging pr: wip PR is still under development area:dmn labels Jan 10, 2025
@jomarko jomarko removed pr: DO NOT MERGE Draft PR, not ready for merging pr: wip PR is still under development labels Jan 23, 2025
@jomarko jomarko marked this pull request as ready for review January 23, 2025 11:00
@jomarko jomarko requested a review from tiagobento as a code owner January 23, 2025 11:00
@kbowers-ibm kbowers-ibm self-requested a review January 28, 2025 10:16
@jomarko jomarko marked this pull request as draft January 28, 2025 10:43
@jomarko
Copy link
Contributor Author

jomarko commented Jan 28, 2025

fixing column resize feature
image

@jomarko
Copy link
Contributor Author

jomarko commented Jan 29, 2025

@tiagobento @kbowers-ibm @ljmotta or anyone else, I will write down my analysis of the problem #2849 (comment), it is the thing that runner output columns are not resizable, there is missing the resize handler. It was introduced by changes regarding readOnly mode https://github.com/apache/incubator-kie-tools/pull/2538/files#diff-ab6fc87d76da643cd7a7f3cd903f077619837939303defc50b2d0e208a2d5ca5.

The missing resize handler in dmn runner outputs columns is a problem because of this two things

Too long header text

Makes header unreadable and in combination with new arrow up button quite 'messy'.
image

Too long data text

Makes data unreadable
image

So here are some options I see. Please comment them or bring other so we can decide what option is the best.

01 'Weak' ReadOnly mode

We could change the isReadOnly of StandaloneBeeTable to be slightly 'weak'. I mean, we could allow to display resize handler even if isReadOnly is set to true

Pros

  • it would fix the issue of unreadable header or data if the text is too long

Cons

  • it decreases the level 'readonly' feeling. for example, readonly in google spreadsheets does not allow to resize column, if I am not wrong
  • it affects more places, not just dmn runner outputs. for example opened included decision node

02 'allowResizing' property

We could introduce additional property allowResizing next to isReadOnly in StandaloneBeeTable

Pros

  • we would be able to fix just the dmn runner outputs scenario without an effect on other StandaloneBeeTable usages

Cons

  • it increases StandaloneBeeTable props complexity

03 'width' computation

We could reuse the column width computation for given text as we have here: https://github.com/apache/incubator-kie-tools/blob/main/packages/dmn-editor/src/boxedExpressions/getDefaultColumnWidth.tsx directly in DmnRunnerOutputsTable when we define beeTableColumns variable.

Pros

  • We do not need to touch StandaloneBeeTable code
  • We keep strong readonly 'feeling' on all StandaloneBeeTable usages

Cons

  • we fix only the scenario of a long text in the header, if a long text is in data cell, it still remains unreadable

@kbowers-ibm
Copy link
Contributor

Thanks for the detailed analysis @jomarko - you've clearly put a lot of thought and effort into this.
My vote would be for Option 3 as I think it would probably be part of any refactoring efforts on the DMN Runner table anyway. This then allows you to continue with your task, and while the con is an issue, it's not like it's introduced by/related to your PR. I think best would be to fix the header as you describe and raise the issue of unreadable long text in data cell as tech debt if it's not already an existing issue.

Copy link
Contributor

@ljmotta ljmotta left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@jomarko Thanks for the PR, I've made some comments inline. Please let me know if you have any questions or need further explanation.

!isLegacyDmnEditor
? (nodeId: string) => {
const newDmnEditorEnvelopeApi = props.editor?.getEnvelopeServer()
.envelopeApi as unknown as MessageBusClientApi<NewDmnEditorEnvelopeApi>;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why the as unknown is required?


export function DmnRunnerTable() {
export function DmnRunnerTable(props: { editor: EmbeddedEditorRef | undefined }) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm don't think we want to have an undefined editor as props of the DmnRunnerTable. Also, the getEnvelopeServer() is the only property of editor that is used, which could be accordlying typed to avoid casting.

@@ -468,6 +468,7 @@ Error details: ${err}`);
workspaceFile={file.workspaceFile}
workspaces={workspaces}
dmnLanguageService={dmnLanguageService}
editor={editor}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As mentioned, we could pass the editorEnvelopeServer.

@@ -186,7 +189,7 @@ export function EditorPageDockContextProvider({
case PanelId.DMN_RUNNER_TABLE:
return (
<DmnRunnerErrorBoundary>
<DmnRunnerTable />
<DmnRunnerTable editor={editor} />
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same.

@@ -99,3 +99,9 @@
font-size: smaller;
color: #1b515f;
}

.kie-tools--bee--header-cell-element-extension {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't want to be picky here, but for this particular case, we don't use the kie-tools--bee prefix. For the boxed-expression-component we are using multiple classes.

Comment on lines +249 to +258
const onOpenBoxedExpressionHeaderButtonClick = useCallback(
(clickedDecisionId: string) => {
(results?.[0] ?? []).flatMap(({ decisionId: resultDecisionId }) => {
if (clickedDecisionId === resultDecisionId) {
openBoxedExpressionEditor?.(resultDecisionId);
}
});
},
[openBoxedExpressionEditor, results]
);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Well, everytime we click on the button we will search the array to check if decisionId is part of the results. Why not save the result decisionIds in a set?

const resultsDecisionIds = useMemo(() => results?.[0]?.reduce((set, result) => {
  set.add(result.decisionId);
  return set;
}, new Set()), [results]);

... 
onClick={() => resultsDecisionIds.has(decisionId) && openBoxedExpressionEditor?.(decisionId)}

(I didn't test the code above)

@ljmotta
Copy link
Contributor

ljmotta commented Jan 29, 2025

@jomarko I agree with @kbowers-ibm . Both option 1 and 2 would require to persist the column size, or closing and opening the DMN Runner Table would reset to the original size.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Sandbox: Add “Open Boxed Expression Editor” button in the DMN Runner table output columns
3 participants