-
Notifications
You must be signed in to change notification settings - Fork 4
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
DSP-638 Value Deletion Comment #198
Conversation
…to provide a comment when deleting a value
@flavens what do you think about this design? Should the label and input be side-by-side or is this okay? |
I would rather use a textarea (you can configure the size to keep it small) because an input may be too short. The label can be the placeholder, instead of being outside of the field. It saves some space. The label could be more straight forward, e.g. "Comment about the deletion" or something like this.. |
…and added a placeholder. Also fixed an error where clicking outside of the dialog box would close the component but try to access an undefined object member.
…s to _viewer file
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.
LGTM
@@ -9,7 +9,12 @@ import { ReadValue } from '@dasch-swiss/dsp-js'; | |||
export class ConfirmationMessageComponent { | |||
|
|||
@Input() value: ReadValue; | |||
comment: string; |
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.
Could the comment by made optional?
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.
made optional in 050a4cd
|
||
export class ConfirmationDialogData { | ||
value: ReadValue; | ||
buttonTextOk: string; | ||
buttonTextCancel: string; | ||
} | ||
|
||
export class ConfirmationDialogPayload { | ||
confirmed: boolean; | ||
deletionComment: string; |
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.
Could the deletionComment
be made optional (instead of setting it to null
when there is no deletion comment)?
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.
made optional in 050a4cd
this._dialogRef.close(true); | ||
const payload = new ConfirmationDialogPayload(); | ||
payload.confirmed = true; | ||
payload.deletionComment = this.confirmationMessageComponent.comment ? this.confirmationMessageComponent.comment : null; |
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.
see my comment above
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.
changed to undefined if a comment is not provided in 050a4cd
confirmed: boolean; | ||
deletionComment: string; | ||
} | ||
|
||
@Component({ | ||
selector: 'dsp-confirmation-dialog', | ||
templateUrl: './confirmation-dialog.component.html', | ||
styleUrls: ['./confirmation-dialog.component.scss'] | ||
}) | ||
export class ConfirmationDialogComponent { |
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.
Is this component specific for value deletion? If so, could its name say so?
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.
renamed in 050a4cd
@@ -937,6 +938,7 @@ describe('DisplayEditComponent', () => { | |||
const deleteVal = new DeleteValue(); | |||
deleteVal.id = 'http://rdfh.ch/0001/H6gBWUuJSuuO-CilHV8kQw/values/dJ1ES8QTQNepFKF5-EAqdg'; | |||
deleteVal.type = 'http://api.knora.org/ontology/knora-api/v2#IntValue'; | |||
deleteVal.deleteComment = null; |
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.
see my comment about optional comments :-)
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.
made optional in 050a4cd
this.deleteValue(); | ||
dialogRef.afterClosed().subscribe((payload: ConfirmationDialogPayload) => { | ||
if (payload && payload.confirmed) { | ||
this.deleteValue(payload.deletionComment); |
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.
What happens if null
is passed?
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.
made optional in 050a4cd
@@ -160,3 +160,30 @@ button.save .mat-icon, | |||
button.cancel .mat-icon { | |||
font-size: 18px; | |||
} | |||
|
|||
.deletion-dialog .title { |
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.
Is this more general than value deletion? Wouldn't it make more sense to keep it in the component's CSS file?
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.
After some thinking, I should refactor this a little. The current ConfirmationMessage component should be renamed and made more specific to a deletion message. This way the ConfirmationDialog component can be reused and you can just supply it with the Message component of your choosing.
Ideally the ConfirmationDialog component is just a shell with a button to confirm and a button to cancel and its contents is defined by the type of message component you give it. We should handle this in another PR though.
Task created https://dasch.myjetbrains.com/youtrack/issue/DSP-781
Should this be unsubscribed from? Edit: The answer is no. I appreciate how simple this guy made it to understand https://stackoverflow.com/a/58199099 |
- renamed ConfirmationDialogPayload to ConfirmationDialogValueDeletionPayload - refactored the deletion comment to be optional so that it will be undefined instead of null if a comment is not provided
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.
Great, thanks for the adaptions.
closes https://dasch.myjetbrains.com/youtrack/issue/DSP-638