BUGFIX: Check for unapplied inspector changes before allowing focus change via <SelectedElement/>
#3501
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
crydotsnake
requested changes
Jun 3, 2023
packages/neos-ui/src/Containers/RightSideBar/Inspector/SelectedElement/index.js
Show resolved
Hide resolved
packages/neos-ui/src/Containers/RightSideBar/Inspector/SelectedElement/index.js
Show resolved
Hide resolved
packages/neos-ui/src/Containers/RightSideBar/Inspector/SelectedElement/index.js
Show resolved
Hide resolved
packages/neos-ui/src/Containers/RightSideBar/Inspector/SelectedElement/index.js
Show resolved
Hide resolved
crydotsnake
approved these changes
Jun 5, 2023
mhsdesign
requested changes
Jun 8, 2023
| @@ -41,6 +41,7 @@ export enum actionTypes { | |||
| DISCARD = '@neos/neos-ui/UI/Inspector/DISCARD', | |||
| ESCAPE = '@neos/neos-ui/UI/Inspector/ESCAPE', | |||
| RESUME = '@neos/neos-ui/UI/Inspector/RESUME', | |||
| ELEMENT_WAS_SELECTED = '@neos/neos-ui/UI/Inspector/ELEMENT_WAS_SELECTED', | |||
There was a problem hiding this comment.
im not sure that i would get without having this image in mind
that we are talking about the little dropdown up top ^^ - its easily forgettable. But then again the naming is scoped to the inspector so i think its still fine.
ill try to think of another name ;)
Comment on lines
+569
to
+575
| const currentlyFocusedNodeContextPaths = | ||
| selectors.CR.Nodes.focusedNodePathsSelector(store.getState()); | ||
|
|
||
| if (currentlyFocusedNodeContextPaths.includes(contextPath)) { | ||
| store.dispatch(actions.CR.Nodes.focus(contextPath, fusionPath)); | ||
| store.dispatch(actions.UI.ContentCanvas.requestScrollIntoView(true)); | ||
| } |
There was a problem hiding this comment.
that seems to be part of the second fix you mentioned that i dont really understand at first glance. But i will try some debugging and see if i can then make sense of those lines plus your description ;)
|
Closed in favor of #3491 |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
fixes: #3174
The problem
#3174 describes a way to bypass the "Unapplied Changes"-dialog that is supposed to prevent the user from losing pending inspector changes.
The user can focus a different node by using the
<SelectedElement/>container that is always at the top of the inspector:This works, because the "Unapplied Changes"-dialog is exclusively triggered by an overlay that appears, when there are pending changes, and covers the entire UI (after #3491 has been merged that is) except the inspector. Since the
<SelectedElement/>container is part of the inspector, it is not covered by the overlay and thus allows the user to bypass the "Unapplied Changes"-dialog.The solution
The first part of the solution introduces a new redux action to the inspector state called
ELEMENT_WAS_SELECTED. (The naming is up for discussion, since it introduces past-tense as opposed to imperatives - which is more correct but runs counter the established convention. It may or may not be a confusing name anyway though - idk)The inspector saga is then used to handle
ELEMENT_WAS_SELECTEDactions so that it first checks if there are pending changes in the inspector, triggers the "Unapplied Changes"-dialog if so, and - depending on the result of the latter - focuses the selected node or leaves the state untouched.The second part of the solution concerns the handling of the
Neos.Neos.Ui:ReloadContentOutOfBandserver feedback. I noticed that if I selected an element and then clicked on "apply" in the "Unapplied Changes"-dialog, the node I've selected would be focused for a brief moment, after which the UI jumped back to the previously focused node.The reason for this was, that the server feedback handler for
Neos.Neos.Ui:ReloadContentOutOfBandwanted to re-focus the node it just reloaded, without checking whether it was still supposed to be focused. I added a check for that, so that no jumping occurs anymore.