Skip to content

feat: Add 'reason' field to move event#6996

Merged
NeilFraser merged 3 commits into
developfrom
fraser-reason
Apr 25, 2023
Merged

feat: Add 'reason' field to move event#6996
NeilFraser merged 3 commits into
developfrom
fraser-reason

Conversation

@NeilFraser
Copy link
Copy Markdown
Contributor

There are many types of move. This addition allows one to detect what the reason for each move is.

There are many types of move.  This addition allows one to detect what the reason for each move is.
@NeilFraser NeilFraser requested a review from a team as a code owner April 20, 2023 18:16
@NeilFraser NeilFraser requested a review from maribethb April 20, 2023 18:16
@github-actions github-actions Bot added the PR: feature Adds a feature label Apr 20, 2023
And unsaved editor tabs.
Comment thread core/events/events_block_move.ts Outdated
* 'cleanup' -- Workspace aligned top-level blocks.
* Reasons may become comma separated if move events merge ('drag,bump,snap').
*/
reason?: string;
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we make the list of reasons an enum? It would mean we get compile-time errors if we e.g. misspell inbounds or try to add a variation like in-bounds, plus give us autocomplete.

Also, can we have the reason property in the event be an array of reasons, instead of a single string with commas? That would be simpler for consumers of this property instead of having to parse the string into an array themselves.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think an enum would allow developers to create new reasons. Imagine an extension or application which, say, shuffled blocks around randomly to create a puzzle. They should be able to create a custom reason (e.g. 'shuffle').

Changed all reasons to be arrays.

* 'inbounds' -- Block got pushed back into a non-scrolling workspace.
* 'connect' -- Block got connected to another block.
* 'disconnect' -- Block got disconnected from another block.
* 'create' -- Block created via XML.
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: Block created via deserialization? (since I assume this would also apply for blocks created json?)

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I assumed the same, but there are no move events fired when deserializing from JSON, just when using XML.

Comment thread core/events/utils.ts Outdated
lastEvent.newCoordinate = moveEvent.newCoordinate;
if (moveEvent.reason) {
if (lastEvent.reason) {
// Contatenate reasons into a comma separated string
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: Concatenate (if this comment still exists pending comment re: using an array)

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done.

@NeilFraser NeilFraser merged commit 64aa3e7 into develop Apr 25, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

PR: feature Adds a feature

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants