Adds sidepanel open and close actions, and gives the user the option to disable click-outside closure of sidepanel #13488
Conversation
…user the option to disable click-outside closure of sidepanel""
There was a problem hiding this comment.
Well done on your first contribution @mikesealey! I've tried to be reasonably verbose here to help explain the changes I've suggested.
Feel free to ask away with any questions if anything isn't clear :)
| @@ -26,6 +29,10 @@ | |||
| } | |||
| } | |||
|
|
|||
| $: { | |||
| sidePanelStore.actions.setSidepanelState(clickOutsideToClose) | |||
There was a problem hiding this comment.
We can't use this store to store a single boolean here, because each side panel has its own setting for whether it can be clicked outside to close or not. By using a single store, multiple side panels will override each other and the last to render will "win" in case where we have multiple side panels (which we do have - like the table autoscreen).
One solution would be only setting this value when a certain side panel actually opens - because all we care about is handling the correct behaviour for the side panel that is currently open, and only one can be open at any given time.
The way you could do this is move this line to be inside the
if (open) {
...
}statement down below.
This way the store is updated to reflect the desired behaviour whenever any side panel is opened.
packages/client/manifest.json
Outdated
| "type": "boolean", | ||
| "key": "clickOutsideToClose", | ||
| "label": "Click outside to close", | ||
| "defaultValue": true |
There was a problem hiding this comment.
We can't default this to true, because all existing apps won't have this value set. For backwards compatibility we need to assume that the default value of any new settings are falsey, and invert the logic.
In this case, this new setting would have to something like "Ignore clicks outside" with a default value of false. This means all old apps will continue to work as normal and the new behaviour is opt in.
packages/client/manifest.json
Outdated
| { | ||
| "type": "event", | ||
| "key": "onSidePanelClose", | ||
| "label": "On side panel close" |
There was a problem hiding this comment.
NAB: We should rename this to just "On close" to keep it in one line.
| @@ -32,11 +33,19 @@ export const createSidePanelStore = () => { | |||
| }, 50) | |||
| } | |||
|
|
|||
| const setSidepanelState = bool => { | |||
There was a problem hiding this comment.
We should use a more descriptive name here - setSidepanelState is very generic and doesn't describe what it's actually doing. I'd recommend something like setIgnoreClicksOutside (if you follow my suggestion for how to rename the setting, otherwise adapt appropriately).
| @@ -32,11 +33,19 @@ export const createSidePanelStore = () => { | |||
| }, 50) | |||
| } | |||
|
|
|||
| const setSidepanelState = bool => { | |||
| clearTimeout(timeout) | |||
There was a problem hiding this comment.
We don't need to clear the timeout here - that's specifically for improving the opening/closing behaviour.
packages/client/manifest.json
Outdated
| @@ -6723,7 +6723,21 @@ | |||
| "illegalChildren": ["section", "sidepanel"], | |||
| "showEmptyState": false, | |||
| "draggable": false, | |||
| "info": "Side panels are hidden by default. They will only be revealed when triggered by the 'Open Side Panel' action." | |||
| "info": "Side panels are hidden by default. They will only be revealed when triggered by the 'Open Side Panel' action.", | |||
| "sendEvents": true, | |||
There was a problem hiding this comment.
Just confirming this is intentional. sendEvents will mean that we will send posthost events whenever this setting is changed by any user. We typically use it to track usage of certain things to improve our default behaviour. Just letting you know that in case this wasn't intentional.
packages/client/manifest.json
Outdated
| }, | ||
| { | ||
| "type": "event", | ||
| "key": "onSidePanelClose", |
There was a problem hiding this comment.
NAB: This can just be called onClose. This is already in the context of the side panel, so we don't need to repeat the name "side panel" inside the setting.
| @@ -51,6 +64,7 @@ | |||
| } else { | |||
| if (target.contains(node)) { | |||
| target.removeChild(node) | |||
| handleSidePanelClose() | |||
There was a problem hiding this comment.
I'm not sure what the default svelte behaviour is here, but I would just double check that this won't invoke the close callback immediately on first render, when the side panel is closed by default.
… closing side panel when navigating away
Re-introduces #13463
PR opened for @aptkingston to comment on, original creator @mikesealey