FIX: Delete Value UI Bug #171
Conversation
| return this._subject$ | ||
| .pipe( | ||
| // Filter down based on event name to any events that are emitted out of the subject from the emit method below. | ||
| filter((e: EmitEvent) => e.name === event), | ||
| map((e: EmitEvent) => e.value) | ||
| map((e: EmitEvent) => e.value), |
|
@mdelez I think I can look at this PR tomorrow. |
| @@ -64,6 +64,8 @@ describe('Test App', () => { | |||
| await browser.wait(EC.presenceOf(element(by.css('.rm-value'))), timeout, | |||
| 'Wait for read value to be visible.'); | |||
|
|
|||
There was a problem hiding this comment.
please add a comment that this is necessary because the DOM element has been re-created
| @@ -17,29 +17,38 @@ export class ValueOperationEventService { | |||
|
|
|||
| // Used in the listening component. | |||
| // i.e. this.valueOperationEventSubscription = this._valueOperationEventService.on(Events.ValueAdded, () => doSomething()); | |||
| on(event: Events, action: (newValue) => void): Subscription { | |||
| on(event: Events, action: (newValue: any) => void): Subscription { | |||
There was a problem hiding this comment.
Instead of any, could you use a more restrictive type annotation?
|
|
||
| /** | ||
| * @param currentValue the current value | ||
| * @param newValue value to update the current value with |
There was a problem hiding this comment.
Could you explain why the current value would have to be updated with the new value?
| @@ -46,7 +46,9 @@ class TestParentComponent implements OnInit, OnDestroy { | |||
| constructor(public _valueOperationEventService: ValueOperationEventService) { } | |||
|
|
|||
| ngOnInit() { | |||
| this.voeSubscription = this._valueOperationEventService.on(Events.ValueAdded, () => this.myNum += 1); | |||
| this.voeSubscription = this._valueOperationEventService.on(Events.ValueAdded, () => this.myNum = 1); | |||
There was a problem hiding this comment.
Wouldn't it make more sense to add the subscriptions to an array of subscriptions?
| @@ -78,21 +78,25 @@ export class ResourceViewComponent implements OnInit, OnChanges, OnDestroy { | |||
| private _valueOperationEventService: ValueOperationEventService) { } | |||
|
|
|||
| ngOnInit() { | |||
| // subscribe to the event bus and listen for the ValueAdded event to be emitted | |||
| // when a ValueAdded event is emitted, get the resource again to display the newly created value | |||
| // subscribe to the ValueOperationEventService and listen for an event to be emitted | |||
| this.valueOperationEventSubscription = this._valueOperationEventService.on( | |||
There was a problem hiding this comment.
see my comment relating to subscriptions
| } | ||
|
|
||
| ngOnChanges() { | ||
| this.getResource(this.iri); | ||
| } | ||
|
|
||
| ngOnDestroy() { | ||
| // unsubscribe from the event bus when component is destroyed | ||
| // unsubscribe from the ValueOperationEventService when component is destroyed | ||
| if (this.valueOperationEventSubscription !== undefined) { |
There was a problem hiding this comment.
this should the rather be an array of subscriptions
|
|
||
| this.valueOperationEventSubscription = this._valueOperationEventService.on( | ||
| Events.ValueDeleted, (deletedValue: DeleteValue) => this.updateResource(deletedValue, true)); | ||
| Events.ValueUpdated, (updatedValue: EventValues) => | ||
| this.updateResource(updatedValue.currentValue, 'update', updatedValue.newValue)); |
There was a problem hiding this comment.
newValue is optional, so how do you know that this is safe?
…n array and the subscriptions are added to it
…smaller methods that are easier to understand
…check if the objectType of a PropertyDefinition is equal to one of the readonly strings defined in the value-type.service for text values
… again after updating a value
|
further optimization of the ValueOperationEvent service will be done in this task https://dasch.myjetbrains.com/youtrack/issue/DSP-628 |
| * @param valueType Value type (ReadValue, DeleteValue, BaseValue, etc.) | ||
| */ | ||
| compareObjectTypeWithValueType(objectType: string, valueType: string): boolean { | ||
| return objectType === this._readTextValueAsString || |
There was a problem hiding this comment.
sorry for the late notice but I think there is some missing conditions:
(valueType === Constants.TextValue && objectType === this._readTextValueAsString) || ...Otherwise text values are always included.
closes https://dasch.myjetbrains.com/youtrack/issue/DSP-550
uses dsp-js 1.0.0.rc10