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

DROOLS-2883: [DMN Designer] Add a Search bar in the Data Type dialog for search top level custom Data Types / DROOLS-3030: [DMN Designer] Expand / Collapse all #2333

Merged
merged 2 commits into from Dec 13, 2018

Conversation

karreiro
Copy link
Contributor

@karreiro karreiro commented Dec 5, 2018

@karreiro karreiro force-pushed the DROOLS-2883 branch 2 times, most recently from 50b9d38 to 2486206 Compare December 5, 2018 18:28
…og for search top level custom Data Types

  * DROOLS-3030: [DMN Designer] Expand / Collapse all
@manstis
Copy link
Member

manstis commented Dec 6, 2018

Jenkins please retest this (now the BPMN2 file indexer test is fixed)

Copy link
Member

@manstis manstis left a comment

Choose a reason for hiding this comment

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

Code looks OK to me; however I can break it quite easily:

  • Launch workbench/webapp
  • Create new DMN file
  • Select "Data Type" tab
  • Add 'a'
  • Add 'b'
  • Search for 'a' ('a' is shown, 'b' is not. Great!). Do not clear/reset search
  • Click Add
  • New item not shown
  • Clear/reset search and they appear (so search is acting more like a "dynamic filter")
  • Select one of the new items created (that was hidden) and "Remove" it
  • "No custom Data Types found" shown. Need to clear/reset search (again)
  • Repeat..
  • Click expand/collapse NPE.

@jomarko
Copy link

jomarko commented Dec 11, 2018

@karreiro postponing my review until you address Michael's comments to avoid spotting the same.

* DROOLS-3329: [DMN Designer] Memory leak in DataTypeList
@karreiro
Copy link
Contributor Author

karreiro commented Dec 11, 2018

Thank you, @manstis.

Regarding the issue when the user adds a new Data Type.. I'm reseting the search bar in this scenario. When users are filtering data types, they are looking for something, and when they are creating a new top-level DT, they don't need to keep the old search.

About the bug related to the "Remove" operation.. the search bar exposed an existing issue, the DROOLS-3329. It's fixed now.

Finally, the NPE with expand/collapse was related to the DROOLS-3329 too.

2018-12-11 11 42 38 am

Thus, changes applied :-) Thanks!


@jomarko Feel free to take a look now ;-)

@jomarko
Copy link

jomarko commented Dec 12, 2018

@karreiro HI, found one minor issue on edge DROOLS-3426. And here are some comments with questions, proposals, let me know if I should file jiras for them.

  • Expand/Collapse doesn't execute action if filtering is active, could we disable (grey color) them when filtering is active
  • The allowed edit actions when filtering is active. It is tightly connected with DROOLS-3015. I see filtering as the way for finding a data type that I want to update in some way. The problem is when I find a data type and update it the result is visible to me just if it mathes the current filter. I think we need to define in more advance level what should happen when user filter for some data type and starts edit it, let me know what do you think.

Otherwise works fine. Tested manually, still need to check code.

Copy link
Member

@manstis manstis left a comment

Choose a reason for hiding this comment

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

Works well now; thanks @karreiro

@jomarko
Copy link

jomarko commented Dec 12, 2018

Code looks fine

@manstis
Copy link
Member

manstis commented Dec 12, 2018

@jomarko If the code is fine and you've tested manually could you please approve the PR?

@karreiro
Copy link
Contributor Author

@manstis Thank you ;-)


@jomarko Thank you!

Regarding the "Expand/Collapse" improvement, I think we can handle this in a separated JIRA.

Also, about the filter behavior (we the user updates a DT), personally I think that the current operation looks like something I expect as a user. But, we can create a UX JIRA to have a broader discussion there about it (but, for now, I think that we're fine).

@manstis manstis merged commit 6b87470 into kiegroup:master Dec 13, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
3 participants