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

feat(DMN): add hints on the returned Java types #286

Merged
merged 1 commit into from
Jan 14, 2019

Conversation

falko
Copy link
Member

@falko falko commented Dec 6, 2018

No description provided.

@ghost ghost assigned falko Dec 6, 2018
@ghost ghost added the needs review Review pending label Dec 6, 2018
@nikku nikku removed the needs review Review pending label Dec 10, 2018
@nikku
Copy link
Member

nikku commented Dec 12, 2018

Thanks for your PR.

Just based on the code you submitted (without any explaination) it is not quite clear what you would you like to achieve. Could you elaborate why this is an improvement (from the user perspective)?

@nikku nikku added the help wanted Extra attention is needed label Dec 12, 2018
@falko
Copy link
Member Author

falko commented Dec 12, 2018

Sorry for my brevity while opening this live during a customer workshop.

In consulting, we frequently experience that customers do not understand the result mapping and what they can expect from the different settings. In our developer trainings, we explain that with the resulting Java types. So the next logical step was to put that type hint also into the labels shown in the property panel. That way users can decide easier and don't have to search the docs or write unit tests in order to figure it out.

@falko
Copy link
Member Author

falko commented Dec 12, 2018

To give even more context:
DMN Decision Tables have different result structures, depending on the Hit Policy. By selecting a mapping that matches the result structure of their model, users can simplify the work with the result later in the process, e.g. in expressions.

@nikku
Copy link
Member

nikku commented Dec 12, 2018

As an alternative to your solution we could simply link the docs, too. Which solution would you prefer?

@falko
Copy link
Member Author

falko commented Dec 12, 2018

Both.

@falko
Copy link
Member Author

falko commented Dec 12, 2018

I guess, this would be the right link to put: https://docs.camunda.org/manual/latest/user-guide/process-engine/decisions/bpmn-cmmn/#predefined-mapping-of-the-decision-result

Nevertheless, we could save people some time by directly putting the hints in the labels, which are not exactly self-explanatory.

@nikku nikku added needs review Review pending and removed needs review Review pending labels Jan 7, 2019
@philippfromme philippfromme merged commit 626151c into bpmn-io:master Jan 14, 2019
@falko
Copy link
Member Author

falko commented Mar 1, 2019

Should this already be available in 3.0.0-nightly builds? I didn't see it there.

@nikku
Copy link
Member

nikku commented Mar 4, 2019

It is in there, however with a display bug:

image

@falko
Copy link
Member Author

falko commented Mar 19, 2019

It is in there, however with a display bug:

image

Any idea how to fix that display bug? Encoding? HTML entities?

@nikku nikku removed the help wanted Extra attention is needed label Mar 19, 2019
@nikku
Copy link
Member

nikku commented Mar 25, 2019

We do not properly sanitize element labels we build the UI from: https://github.com/bpmn-io/bpmn-js-properties-panel/blob/master/lib/factory/SelectEntryFactory.js#L51

This means that entities such as < have interesting effects on the UI.

@nikku nikku mentioned this pull request Mar 25, 2019
4 tasks
@nikku
Copy link
Member

nikku commented Mar 25, 2019

I've created #296 to track the bug.

@falko
Copy link
Member Author

falko commented Apr 9, 2019 via email

@nikku
Copy link
Member

nikku commented Apr 9, 2019

Unfortunately not. We scheduled #296 this as a bug in our next milestone.

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

Successfully merging this pull request may close these issues.

3 participants