-
Notifications
You must be signed in to change notification settings - Fork 218
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-2653: [DMN Designer] Replace 'My name' (etc) with better defaults #1929
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Basically, there is no issue in this PR and in rapidly improves user experience. However there is inconsistency between DRG and expression editors.
- In DRG new element name is built on top of count of nodes
- In expression new element name is build on the suffix , is it intentional?
I mean for elements : abc-3 and abc-5 the DRG will add element with name abc-3 while expression will add element with name abc-6 is my question clear?
} else if (dmnModel instanceof TextAnnotation) { | ||
updateTextAnnotationDefaultText(graph, (TextAnnotation) dmnModel); | ||
} else { | ||
throw new IllegalArgumentException("Default Name for '" + dmnModel.getClass().getSimpleName() + "' is not support."); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should be the error message .... is not supported. ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes it should! Your English is brilliant.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No it is not and we know it :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
thanks!
Jenkins please retest this. |
* DROOLS-2653: Reduce codebase * DROOLS-2653: Reduce Codebase. Post review actions. * DROOLS-2653: Redice Codebase. Minor changes following jomarko updates.
…lts. Updates following peer review.
@jomarko I have merged your codebase reduction and resolved the inconsistency between "graph node names" and "expression element names". Both now work the same; being a lookup for a next unused index value (rather than node count). So, given a node "abc-5" the next will be "abc-6". |
@karreiro Could you please review? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you, @manstis.
I noticed some nodes being displayed like this:
In the future, we could improve the readability by maybe adding a white background, something like this:
..but this is just an idea. If you think that's a good one, we could raise a JIRA for that ;-)
…lts (kiegroup#1929) * DROOLS-2653: [DMN Designer] Replace 'My name' (etc) with better defaults * DROOLS-2653: Reduce codebase (kiegroup#31) * DROOLS-2653: Reduce codebase * DROOLS-2653: Reduce Codebase. Post review actions. * DROOLS-2653: Redice Codebase. Minor changes following jomarko updates. * DROOLS-2653: [DMN Designer] Replace 'My name' (etc) with better defaults. Updates following peer review.
See https://issues.jboss.org/browse/DROOLS-2653