Feature/finished/iia 1896 added confirm reset dialog #537
Conversation
…existing clear confirmation dialog
jdsmithsos
requested review from
bhharsh13,
carldea,
dholubek,
dukke and
swaroopsalvi
bhharsh13
left a comment
bhharsh13
left a comment
There was a problem hiding this comment.
small text fix. Rest looks good to me
| doTheClearOrResetForm(); | ||
| ConfirmationDialogController.showConfirmationDialog(this.cancelButton, | ||
| "Confirm Reset Form", | ||
| "You're changes will be lost if you reset the form. Are you sure you want to continue?") |
There was a problem hiding this comment.
requires correction in the message displayed - Your
There was a problem hiding this comment.
requires correction in the message displayed - Your
done
dholubek
left a comment
dholubek
left a comment
There was a problem hiding this comment.
had one minor request, but looks fine to me.
| /** | ||
| * Creates the dialog, providing the CompletableFuture that provides the modal behavior of the dialog. | ||
| * @param parentNode The node to start at to determine the topmost Pane, which is required for the GlassPane | ||
| * @return CompletableFuture with a Boolean type, Boolean value of true is returned when the confirm button is pressed |
There was a problem hiding this comment.
can we add the other two method arguments to the javadoc also?
There was a problem hiding this comment.
can we add the other two method arguments to the javadoc also?
done
swaroopsalvi
left a comment
swaroopsalvi
left a comment
There was a problem hiding this comment.
Have minor comments.
|
|
||
| final JFXNode<Pane, ConfirmClearDialogController> githubInfoNode = FXMLMvvmLoader | ||
| .make(ConfirmClearDialogController.class.getResource("confirm-clear-dialog.fxml")); | ||
| final JFXNode<Pane, ConfirmationDialogController> githubInfoNode = FXMLMvvmLoader |
There was a problem hiding this comment.
why is this named as githubInfoNode ? shouldn't it be javaFxNode instead ? Can we change this variable name?
There was a problem hiding this comment.
This was from copy and paste. Renamed to confDialogNode.
| } else { | ||
| doTheClearOrResetForm(); | ||
| ConfirmationDialogController.showConfirmationDialog(this.cancelButton, | ||
| "Confirm Reset Form", |
There was a problem hiding this comment.
For consistency in code, can we declare the text as final like we did for CLEAR ? or make both local to the if -else conditions.
There was a problem hiding this comment.
Chose not to use static final here because this text is only used in one place for reset.
The clear text for title and message are used in two places in the code, so static final is necessary to use the same text in both places.
jdsmithsos
deleted the
feature/finished/IIA-1896-added-confirm-reset-dialog
branch
4 participants
Jira ticket: https://ikmdev.atlassian.net/browse/IIA-1896
Summary of changes:
Recommend expanding on this an adding a refactor story to create a common, global, dialog class that is responsible for creating the standard Komet application dialog which uses the GlassPane.
Before change:
No Confirm Reset dialog.
After change:
Confirm Reset dialog is displayed.