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-3930: Improve usability from Guided Rule Template - Can't resize column #2600
Conversation
@@ -82,27 +87,54 @@ | |||
private DynamicColumn<T> resizeColumn = null; | |||
private int resizeColumnWidth = 0; | |||
|
|||
public boolean isResizePrimed() { |
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.
Some more getters and setters to an inner class to help with Unit Testing.
tce.appendChild( div ); | ||
tre.appendChild( tce ); | ||
getBody().appendChild( tre ); | ||
TableRowElement tre = Document.get().createTRElement(); |
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.
Tweaks to how the DOM
elements are created for Unit Tests.
|
||
// Add the resizer to the outer most container, otherwise it gets | ||
// truncated by the ScrollPanel as it hides any overflow | ||
div.appendChild( resizer ); |
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.
Don't attach the "column resize handlers" to the header widget........
} | ||
|
||
@Override | ||
protected void onLoad() { |
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.
... but attach them to the RootPanel
(whole Document
) when this Widget
is attached to the DOM
.
if ( handler == null ) { | ||
throw new IllegalArgumentException( "handler cannot be null" ); | ||
@Override | ||
protected void onUnload() { |
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.
Make sure the "column resize handlers" are released when this Widget
is removed from the DOM
.
MouseOutEvent.getType())); | ||
} | ||
|
||
RootPanel rootPanel() { |
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.
Allow the RootPanel
to be mocked in Unit Tests.
void setResizerDimensions(final int position) { | ||
resizer.getStyle().setHeight(parent.getElement().getClientHeight(), | ||
Unit.PX); | ||
resizer.getStyle().setLeft(position - panel.getAbsoluteLeft(), |
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.
Tweak for x-coordinate calculation now we're adding the event handlers to a parent panel.
import static org.mockito.Mockito.when; | ||
|
||
@RunWith(GwtMockitoTestRunner.class) | ||
public class AbstractDecoratedGridHeaderWidgetColumnResizeTest { |
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.
Maybe not exhaustive but some Unit Tests (for the "happy path") are better than none!
jenkins please retest this |
Manual review notesWorks fine from my point of view, just one question, column width is not preserved even between clicking different tabs (Model, Source, ...) Is it expected? I will proceed to code 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.
Code review
Just one note about code duplication.
.../workbench/common/widgets/decoratedgrid/client/widget/AbstractDecoratedGridHeaderWidget.java
Outdated
Show resolved
Hide resolved
…ze column. Updates following peer review.
@jomarko The editor has never preserved column width between switching tabs.. nobody has asked for it so I am reluctant to add such support to this PR. Persisting the column width requires even more extensive changes and to be honest unless a request comes through as a RFE I'd be reluctant to consider it (on a deprecated editor that really has no support other than customer tickets). |
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 for answer and updates!
See https://issues.jboss.org/browse/DROOLS-3930
Unfortunately _lots_of formatting changes; but I'll call out the main changes below.