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-2913: [DMN Designer] Data-types: Grid: FunctionDefinition #2055

Merged
merged 2 commits into from Aug 28, 2018
Merged

DROOLS-2913: [DMN Designer] Data-types: Grid: FunctionDefinition #2055

merged 2 commits into from Aug 28, 2018

Conversation

manstis
Copy link
Member

@manstis manstis commented Aug 22, 2018

See https://issues.jboss.org/browse/DROOLS-2913

As with all these "in grid" editor changes; this PR will be merged after the others in the queue.

Copy link
Contributor

@karreiro karreiro left a comment

Choose a reason for hiding this comment

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

Thank you, @manstis.


Only one non-blocking comment ;-)

protected Consumer<HasName> clearDisplayNameConsumer() {
return (hn) -> {
final CompositeCommand.Builder commandBuilder = newHasNameHasNoValueCommand(hn);
getUpdateStunnerTitleCommand("").ifPresent(commandBuilder::addCommand);
Copy link
Contributor

Choose a reason for hiding this comment

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

I think that I already saw something pretty similar on InvocationGrid, LiteralExpression, and BaseExpressionGrid. Maybe we could try to avoid the duplication, but if the refactoring represents a big indirection, I'm ok for keeping it as is.

Copy link
Member Author

Choose a reason for hiding this comment

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

@karreiro Let me loop back to this type of thing when all the current PRs are merged (i.e. I'll collect all these types of comment and probably do a "clean up" JIRA afterwards). Thank-you.

Copy link
Member Author

Choose a reason for hiding this comment

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

@karreiro ... and yes, this standard override of the default implementation is re-used in the places you say... but not everywhere; only overrides in a few places (but the override is the same each time, so room for improvement).

Copy link
Contributor

@karreiro karreiro Aug 22, 2018

Choose a reason for hiding this comment

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

@manstis ok. Thank you for clarifying 😊

Copy link
Member Author

Choose a reason for hiding this comment

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

@manstis
Copy link
Member Author

manstis commented Aug 24, 2018

@jomarko This PR is good to review too now; thanks.

@jomarko
Copy link

jomarko commented Aug 24, 2018

@manstis will do after done with changes in #1889

@ederign
Copy link
Member

ederign commented Aug 27, 2018

@jomarko can you please take a look on this?

final Text typeRef = context.getRenderer().getTheme().getHeaderText();
typeRef.setFontStyle(FONT_STYLE_TYPE_REF);
typeRef.setFontSize(BaseExpressionGridTheme.FONT_SIZE - 2.0);
typeRef.setText("(" + headerMetaData.getTypeRef().toString() + ")");
Copy link

Choose a reason for hiding this comment

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

I think this can be shortened to:

typeRef.setText("(" + headerMetaData.getTypeRef() + ")");

Copy link
Member Author

Choose a reason for hiding this comment

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

@jomarko Updated.

@jomarko
Copy link

jomarko commented Aug 28, 2018

@manstis manual check passed, please just say your opinion to my comment above before approving.

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.

Thanks!

@manstis
Copy link
Member Author

manstis commented Aug 28, 2018

Jenkins please retest this.

1 similar comment
@manstis
Copy link
Member Author

manstis commented Aug 28, 2018

Jenkins please retest this.

Copy link
Member

@ederign ederign left a comment

Choose a reason for hiding this comment

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

failing test is not related.

@ederign ederign merged commit f18635c into kiegroup:master Aug 28, 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
4 participants