Adds the option to Clear Selected Records #73
Conversation
Added to the Clear Menu, this option clears the selected records from Table and graphics related to it.
| layer.remove(graphic); | ||
| }); | ||
|
|
||
| topic.publish(this.attributesContainerID + '/recordsRemoved', layer.graphics); |
There was a problem hiding this comment.
Had some doubts about it (the publish of /recordsRemoved), but since I needed to fire some event about it, I decided to publish this new topic.
There was a problem hiding this comment.
I think this topic should be /graphicsRemoved and a separate topic rowsRemoved published in the clearSelectedGridRecords method (or the proposed name clearSelectedGridRows) for publishing the grid rows that were removed.
There was a problem hiding this comment.
Agree with everything. Due to my needings, I was publishing remaining graphics. Do I keep it? Remaining/removed graphics/rows?
There was a problem hiding this comment.
oh right. I was looking at that backwards. To be consistent, how about publishing the topic like this as it is done elsewhere:
topic.publish(this.attributesContainerID + '/tableUpdated', this);It would then only need to be called once instead of in the two places as I had suggested.
I recognize your code elsewhere would need to use something like args.featureGraphics.graphics instead of just args
There was a problem hiding this comment.
Since _FeaturesMixin's clearFeatures already publishes tableUpdated, I guess I don't need to publish it again. In my specific version, I'll keep publishing those custom topics.
|
Thanks @carrbrpoa! Will review when I am in the office. |
| } | ||
| this.setToolbarButtons(); | ||
| topic.publish(this.attributesContainerID + '/tableUpdated', this); | ||
|
|
||
| }, | ||
|
|
||
| clearSpecificFeatures: function (layer, specificFeatures) { |
There was a problem hiding this comment.
this method should be named clearSpecificGraphics since it is not specific to Features and can be used for any of the graphic layers.
| @@ -648,15 +648,45 @@ define([ | |||
| this.clearGraphicsLayer(this.bufferGraphics); | |||
| }, | |||
|
|
|||
| clearGraphicsLayer: function (layer) { | |||
| clearGraphicsLayer: function (layer, specificFeatures) { | |||
There was a problem hiding this comment.
the argument should be specificGraphics so it can be generalized for any graphic layer.
| layer.remove(graphic); | ||
| }); | ||
|
|
||
| topic.publish(this.attributesContainerID + '/recordsRemoved', layer.graphics); |
There was a problem hiding this comment.
I think this topic should be /graphicsRemoved and a separate topic rowsRemoved published in the clearSelectedGridRecords method (or the proposed name clearSelectedGridRows) for publishing the grid rows that were removed.
| @@ -302,6 +302,15 @@ define([ | |||
| this.gridOptions.sort = []; | |||
| }, | |||
|
|
|||
| clearSelectedRecords: function () { | |||
There was a problem hiding this comment.
Should method be renamed to clearSelected or similar since it included graphics and isn't specific to just dgrid rows or dstore records?
| @@ -419,6 +419,29 @@ define([ | |||
| } | |||
| this.setToolbarButtons(); | |||
| topic.publish(this.attributesContainerID + '/tableUpdated', this); | |||
| }, | |||
|
|
|||
| clearSelectedGridRecords: function () { | |||
There was a problem hiding this comment.
I think this should be clearSelectedGridRows to be consistent with dgrid's use of rows, not records in a dstore.
|
@carrbrpoa Thank you for the contribution! |
Added to the Clear Menu, this option clears the selected records from
Table and graphics related to it.
This PR is related to issue 71.