More detailed commandStack events #486
Conversation
|
|
|
Is this an issue with my code? I don't see any problems |
lib/command/CommandStack.js
Outdated
|
|
||
| dirty.length = 0; | ||
|
|
||
| this._fire('changed'); | ||
| this._fire('changed', assign({ method: method, actions: execution.executed }, { context })); |
There was a problem hiding this comment.
I guess the build is failing because you use unsupported JS syntax.
{ context }The shorthand object initializer is an es6 feature and therefore not available for older browsers. Better would be
{ context: context }
|
Thanks a lot first for your contribution! I appreciate it very much that you're trying to implement this feature on your own. 👍 We will discuss in the team how to handle your proposal. In the meantime, can you change your pull request structure to fit our guidelines? Especially test cases which test the new features and a formatted git commit message would be very appreciated. |
| // Need to figure out a way to indicate the executed actions | ||
| expect(actions).to.eql(['pre-command', 'complex-command', 'post-command']); |
There was a problem hiding this comment.
This one has an issue though, that I discovered when making tests (I skipped the relevant test because it fails). The redo action doesn't indicate the executed actions. I can't figure out a graceful way to do this, so I would appreciate a nudge in the right direction!
|
I am closing this until we figure out what we actually want to have. |
|
I have amended the pull-request as I described in the related issue. Now it will only indicate the change method ( I see however that the pull-request didn't update. Is this because it is closed? |
Apparently you force pushed or updated the develop branch, that is why you cannot reopen the PR. |
Closes #479
I haven't rigorously tested it, but it seems stable for my usage. I have only used the execute method, but I went ahead and added undo, redo and clear to make it a complete feature.