Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

BUGFIX: Check for unapplied inspector changes before allowing focus change via <SelectedElement/> #3501

Closed
wants to merge 2 commits into from
Closed

BUGFIX: Check for unapplied inspector changes before allowing focus change via <SelectedElement/> #3501

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
73c8027
BUGFIX: Check for unapplied inspector changes before allowing focus c…
grebaldi May 22, 2023
e71939a
BUGFIX: Re-focus reloaded content node only if it is still focused
grebaldi May 22, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
8 changes: 7 additions & 1 deletion packages/neos-ui-redux-store/src/UI/Inspector/index.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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',
Copy link
Member

Choose a reason for hiding this comment

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

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 ;)


//
// Actions to control the secondary inspector window
Expand All @@ -61,6 +62,10 @@ const apply = () => createAction(actionTypes.APPLY);
const discard = (focusedNodeContextPath: NodeContextPath) => createAction(actionTypes.DISCARD, {focusedNodeContextPath});
const escape = () => createAction(actionTypes.ESCAPE);
const resume = () => createAction(actionTypes.RESUME);
const selectElement = (selectedElementContextPath: NodeContextPath) =>
crydotsnake marked this conversation as resolved.
Show resolved Hide resolved
createAction(actionTypes.ELEMENT_WAS_SELECTED, {
selectedElementContextPath,
});

const openSecondaryInspector = () => createAction(actionTypes.SECONDARY_OPEN);
const closeSecondaryInspector = () => createAction(actionTypes.SECONDARY_CLOSE);
Expand All @@ -76,9 +81,10 @@ export const actions = {
discard,
escape,
resume,
selectElement,
crydotsnake marked this conversation as resolved.
Show resolved Hide resolved
openSecondaryInspector,
closeSecondaryInspector,
toggleSecondaryInspector
toggleSecondaryInspector,
};

export type Action = ActionType<typeof actions>;
Expand Down
53 changes: 47 additions & 6 deletions packages/neos-ui-sagas/src/UI/Inspector/index.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -30,6 +30,7 @@ export function * inspectorSaga({globalRegistry}) {
const waitForNextAction = yield race([
take(actionTypes.CR.Nodes.FOCUS),
take(actionTypes.UI.ContentCanvas.SET_SRC),
take(actionTypes.UI.Inspector.ELEMENT_WAS_SELECTED),
take(actionTypes.UI.Inspector.DISCARD),
take(actionTypes.UI.Inspector.APPLY)
]);
Expand Down Expand Up @@ -63,6 +64,45 @@ export function * inspectorSaga({globalRegistry}) {
break;
}

//
// If the user used the <SelectedElement/> Dropdown, show the
// UnappliedChangesDialog and react accordingly
//
if (nextAction.type === actionTypes.UI.Inspector.ELEMENT_WAS_SELECTED) {
yield put(actions.UI.Inspector.escape());
const [escapeAction] = Object.values(
yield race([
take(actionTypes.UI.Inspector.RESUME),
take(actionTypes.UI.Inspector.APPLY),
take(actionTypes.UI.Inspector.DISCARD)
])
);

if (escapeAction.type === actionTypes.UI.Inspector.RESUME) {
// The user has decided to continue editing properties in the inspector
break;
}

if (escapeAction.type === actionTypes.UI.Inspector.APPLY) {
try {
yield * applyInspectorChanges(inspectorRegistry);
} catch (err) {
//
// An error occured, we should not leave the loop until
// the user does something about it
//
console.error(err);
continue;
}
}

yield put(actions.CR.Nodes.focus(nextAction.payload.selectedElementContextPath));

if (escapeAction.type === actionTypes.UI.Inspector.DISCARD) {
break;
}
}

//
// If the user discarded his/her changes, just continue
//
Expand All @@ -75,12 +115,7 @@ export function * inspectorSaga({globalRegistry}) {
//
if (nextAction.type === actionTypes.UI.Inspector.APPLY) {
try {
//
// Persist the inspector changes
//
yield call(flushInspector, inspectorRegistry);
const focusedNodeContextPath = yield select(selectors.CR.Nodes.focusedNodePathSelector);
yield put(actions.UI.Inspector.clear(focusedNodeContextPath));
yield * applyInspectorChanges(inspectorRegistry);
} catch (err) {
//
// An error occured, we should not leave the loop until
Expand All @@ -96,6 +131,12 @@ export function * inspectorSaga({globalRegistry}) {
}
}

function * applyInspectorChanges(inspectorRegistry) {
yield call(flushInspector, inspectorRegistry);
const focusedNodeContextPath = yield select(selectors.CR.Nodes.focusedNodePathSelector);
yield put(actions.UI.Inspector.clear(focusedNodeContextPath));
}

function * flushInspector(inspectorRegistry) {
const state = yield select();
const focusedNode = getFocusedNode(state);
Expand Down
8 changes: 4 additions & 4 deletions packages/neos-ui/src/Containers/RightSideBar/Inspector/SelectedElement/index.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -23,22 +23,22 @@ import style from './style.css';
focusedNodeParentLine: selectors.CR.Nodes.focusedNodeParentLineSelector(state)
};
}, {
focusNode: actions.CR.Nodes.focus
selectElement: actions.UI.Inspector.selectElement
crydotsnake marked this conversation as resolved.
Show resolved Hide resolved
})
export default class SelectedElement extends PureComponent {
static propTypes = {
focusedNode: PropTypes.object.isRequired,
focusedNodeParentLine: PropTypes.array.isRequired,

focusNode: PropTypes.func.isRequired,
selectElement: PropTypes.func.isRequired,
crydotsnake marked this conversation as resolved.
Show resolved Hide resolved
nodeTypesRegistry: PropTypes.object.isRequired
};

handleSelectNode = selectedNodeContextPath => {
const {focusNode, focusedNode} = this.props;
const {selectElement, focusedNode} = this.props;
crydotsnake marked this conversation as resolved.
Show resolved Hide resolved

if (selectedNodeContextPath && selectedNodeContextPath !== $get('contextPath', focusedNode)) {
focusNode(selectedNodeContextPath);
selectElement(selectedNodeContextPath);
crydotsnake marked this conversation as resolved.
Show resolved Hide resolved
}
};

Expand Down
10 changes: 8 additions & 2 deletions packages/neos-ui/src/manifest.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -565,8 +565,14 @@ manifest('main', {}, globalRegistry => {
nodes
})
);
store.dispatch(actions.CR.Nodes.focus(contextPath, fusionPath));
store.dispatch(actions.UI.ContentCanvas.requestScrollIntoView(true));

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));
}
Comment on lines +569 to +575
Copy link
Member

Choose a reason for hiding this comment

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

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 ;)

});

//
Expand Down