Skip to content
This repository has been archived by the owner on Apr 12, 2022. It is now read-only.

KOGITO-3656/KOGITO-5649: Support copy/paste integration with other spreadsheets / Support single/multiple cells highlight #103

Merged
merged 1 commit into from Sep 20, 2021

Conversation

karreiro
Copy link
Contributor

@karreiro karreiro commented Sep 8, 2021

JIRAs:

  • KOGITO-3656: [DMN Designer] New boxed expression editor - Support copy/paste integration with other spreadsheets
  • KOGITO-5649: [DMN Designer] New boxed expression editor - Support single/multiple cells highlight

Here's the live demo: karreiro.com/kogito-editors-java


@jomarko
Copy link

jomarko commented Sep 9, 2021

01 rename column

We have name and data type for columns. however currently I am not able to rename any column. Is that related to this PR?

@jomarko
Copy link

jomarko commented Sep 9, 2021

02 multiple cells selection

If I understand correctly from the gif, I should be able to select multiple cells (of my decision table for example) if I simply drag a rectangle with mouse.

If that is correct, I struggle to use this feature. Was able to select desired area in 1/10 attempts. The selection feature was in most cases even not started or it was stopped preliminary.

Is this dependent on some OS Mouse Speed settings? Is some keyboard key needed to use the selection feature?

@jomarko
Copy link

jomarko commented Sep 9, 2021

03 render after paste

Try to paste some values into Relation expression, however not into [0, 0] cell. The UI will be not rendered with pasted values, however they will be already shown in the json presenter bellow.

Also the "Define 50x50 Relation expression" test is failing. On the main branch the test passes.

@jomarko
Copy link

jomarko commented Sep 9, 2021

04 function

If there is java function expression, and user tries to paste are bigger than single cell into class, the attached will happen.
Screenshot from 2021-09-09 09-23-44

Either no acttion should be taken or just single value from the copied area should be pasted I think. No context entries should appear in the expressions.

@jomarko
Copy link

jomarko commented Sep 9, 2021

05 invocation

Very similar issue as 04

@jomarko
Copy link

jomarko commented Sep 9, 2021

06 list

Very similar issue as 04

@jomarko
Copy link

jomarko commented Sep 9, 2021

07 empty cells

Empty cells in the copied area behavior is different from google spreadsheets

Original content in DMN editor
|a|a|
|b|b|
|c|c|

Copied area
|1|1|
| | |
|2|2|

the result in DMN editor
|1|1|
|b|b|
|2|2|

the result in google spreadsheets
|1|1|
| | |
|2|2|

Copy link

@jomarko jomarko left a comment

Choose a reason for hiding this comment

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

@karreiro thank you for the PR. It is a huge improvement. However I can not approve due tho the comments above.

@jomarko
Copy link

jomarko commented Sep 9, 2021

@karreiro I tried also to write some cypress test with t do sanity check of copy and paste, using this paste custom command [1], however I am not sure, what element should I locate/what element should consume the paste event. Could you advise?

[1]
https://gist.github.com/nickytonline/bcdef8ef00211b0faf7c7c0e7777aaf6#gistcomment-3305442

Copy link
Contributor

@vpellegrino vpellegrino left a comment

Choose a reason for hiding this comment

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

The code looks great, with minor comments.
I am waiting for you to solve the issues that @jomarko raised before performing manual tests.

Since the boxed-expression-component can be integrated anywhere, maybe it's better to set the scope of DOM operations on the boxedExpressionEditorRef instead of document, WDYT?

const globalContext = useContext(BoxedExpressionGlobalContext);
globalContext.boxedExpressionEditorRef?.current;
```1) Should `dmn-runner-showcase` folder be inside `boxed-expression-component`? Logically it is a self-contained project that *uses* the `boxed-expression-component`. Any technical reason for not being an isolated project which has `boxed-expression-component` as a dependency?

@kie-ci
Copy link

kie-ci commented Sep 14, 2021

The (build) Kogito Tooling Editors Java check has failed. Please check the logs.

@karreiro
Copy link
Contributor Author

karreiro commented Sep 14, 2021

Thank you for the review @jomarko and @vpellegrino. I've fixed all issues mentioned above. Also, I've improved the selection mechanism (following the item 02 multiple cells selection), now it's easier to select multiple elements:

demo

The live demo (in the PR description) is updated, and this PR is ready for the re-review. 🚀

Thanks again for the feedback!

@jomarko
Copy link

jomarko commented Sep 15, 2021

02_1

Paste into context with single entry causes all the generated entries has same name. Not sure what is the best solution here:

  • prevent such paste
  • generate unique context entries keys
  • keep it as it is

Screenshot from 2021-09-15 13-29-33

@jomarko
Copy link

jomarko commented Sep 15, 2021

02_2

Please follow this scenario

  • create relation with two columns and single row
  • paste area of two columns and few rows
  • rows were added into relation what is perfect
  • try to insert row in the middle of new area, the issue is not all cells of the added row will be empty

Screenshot from 2021-09-15 13-35-53
Screenshot from 2021-09-15 13-36-08
Screenshot from 2021-09-15 13-36-27

@jomarko
Copy link

jomarko commented Sep 15, 2021

02_3

I can still reproduce #103 (comment)

@jomarko
Copy link

jomarko commented Sep 15, 2021

02_4

  • prepare relation with single column, few rows
  • select the rows
  • copy them: ctrl+c
  • insert column left
  • try to paste copied values into new column

Screenshot from 2021-09-15 13-46-19

Screenshot from 2021-09-15 13-49-56

KOGITO-5649: Support single/multiple cells highlight
@karreiro
Copy link
Contributor Author

Thanks for the review, @jomarko. I've answered each topic inline:

  • 02_1 - As this is not something that blocks users and they are able to successfully rename rows. I've raised this Jira (KOGITO-5920) to fix it in another PR.

  • 02_2 - This issue is already happening in the main branch. So, I've raised this Jira (KOGITO-5922) to fix it in another PR.

  • 02_3 - Well spotted, thanks! It's fixed now.

  • 02_4 - I cannot reproduce this issue in the most updated version of the editor. Is it still happening?

The live demo (in the PR description) is updated, and this PR is ready for the re-review.

Thanks again for the feedback!

@sonarcloud
Copy link

sonarcloud bot commented Sep 16, 2021

Kudos, SonarCloud Quality Gate passed!    Quality Gate passed

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 0 Code Smells

No Coverage information No Coverage information
No Duplication information No Duplication information

Copy link

@jomarko jomarko left a comment

Choose a reason for hiding this comment

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

@karreiro thank you. The 02_4 is still reproducible, I reported it separately [1] as I have PTO until 28th of September to not block merge of this.

[1]
https://issues.redhat.com/browse/KOGITO-5928

@karreiro
Copy link
Contributor Author

@vpellegrino Could you please take a look at this PR? :-)

Copy link
Contributor

@vpellegrino vpellegrino left a comment

Choose a reason for hiding this comment

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

LGTM,

thanks @karreiro

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
4 participants