-
Notifications
You must be signed in to change notification settings - Fork 322
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.
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
feat: Adding support for altering and overriding overlay actions #558
feat: Adding support for altering and overriding overlay actions #558
Conversation
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
@xaviemirmon is attempting to deploy a commit to the Measured Team on Vercel. A member of the Team first needs to authorize it. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is great! I've left a bunch of comments, but have also been riffing on this and playing with some more ideas taking the essence of what you've done.
Fundamentally, I think we should:
- Call this permissions instead of overlayActions to reflect it can be used for other things (like disabling the form)
- Avoid putting permission info in the Data object and use the UI state instead to avoid bloating the user payload
- Enable the user to specify global permissions at the
<Puck>
level - Enable the user to specify component permissions at the component level
- Enable the user to specify instance (local) permissions based on the ID
- Provide some helpers for getting and setting permissions via
usePuck
- Enable overriding of the entire overlayAction bar (which I think should be called the
actionBar
override).
I think we can probably split this work into 2 PRs:
- The core permissions API and usePuck enhancements
- The overrides API and associated component exports
Here's a fully fledged example that describes what I'm thinking:
"use client";
import { ActionBar, Puck, usePuck } from "@/core";
import { Lock, Unlock } from "lucide-react";
const data = {};
export function Editor() {
const permissions = {
duplicate: false,
edit: true,
// A custom permission for the user
party: true,
};
return (
<Puck
// Specify global permissions
initialPermissions={permissions}
// User doesn't need to specify this unless they want to load previous state, but this gives an idea of shape
ui={{
permissions: {
// Global permissions
global: {
delete: true, // Add the default, as not specified by user
duplicate: false,
edit: true,
party: true,
},
// Component permissions, as specified by a component config
// Inherits global
component: {
MyComponent: {
edit: false,
},
},
// Permissions for specific instances of a component
// Inherits component
local: {
"MyComponent-1234": {
edit: true,
},
},
},
}}
config={{
components: {
MyComponent: {
permissions: {
edit: false,
},
},
},
}}
data={data}
overrides={{
// Enable override of entire action bar, not just internal actions
actionBar: ({ children, item }) => {
const {
// New API to get and mutate permissions on UI
getPermission,
setPermission,
} = usePuck();
const { id, type } = item.props;
// Get resolved permission, with local first, then component, then global.
const isEditable = getPermission({ permission: "edit", id, type });
return (
// <ActionBar> is the default UI
<ActionBar>
<div
style={{
display: "flex",
}}
>
{isAdmin && (
<ActionBar.Action
onClick={() => {
setPermission(
{
permission: "edit",
id, // Set local permission for this ID
type, // Set component permission for this type
// Or set global permission if neither specified
},
!isEditable
);
// setPermission modifies state internally 👇
// dispatch({
// type: "setUi",
// ui: {
// permissions: {
// local: {
// [id]: {
// edit: !isEditable,
// },
// },
// },
// },
// });
}}
>
{!isEditable ? <Unlock size={16} /> : <Lock size={16} />}
</ActionBar.Action>
)}
</div>
{children}
</ActionBar>
);
},
}}
/>
);
}
{children} | ||
</> | ||
), | ||
overlayActions: ({ children, state, dispatch }) => { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We don't need state
or dispatch
here, but we need the itemSelector
(and ideally actual item
) for the currently hovered item.
This will often be different from the itemSelector
or selectedItem
present in the UI state, as the overlay appears on hover but the properties in the UI state represent the item currently selected.
children: ReactNode; | ||
state: AppState; | ||
dispatch: (action: PuckAction) => void; | ||
}>; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We don't need to pass in state
or dispatch
here - the user can use the usePuck
hook
@@ -28,6 +30,11 @@ export type Overrides = OverridesGeneric<{ | |||
fieldTypes: Partial<FieldRenderFunctions>; | |||
header: RenderFunc<{ actions: ReactNode; children: ReactNode }>; | |||
headerActions: RenderFunc<{ children: ReactNode }>; | |||
overlayActions: RenderFunc<{ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I reckon this should be replaced with an actionBar
override, for overriding the entire bar (including the background, with an additional <ActionBar>
component export so the user can render the default:
actionBar: ({children}) => {
return (<ActionBar>
<button />
{children}
</ActionBar>);
}
@@ -105,12 +114,14 @@ export type ComponentData< | |||
> = { | |||
type: keyof Props; | |||
props: WithPuckProps<Props>; | |||
overlayActions?: OverlayActions; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think this should exist in the data payload - this is application state
() => loadedOverrides.overlayActions || defaultRender, | ||
[loadedOverrides] | ||
); | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Perhaps we could spin this actions behaviour out into an explicit component - that will likely help when I rebase #556, which totally guts this component.
}} | ||
> | ||
<button | ||
className={"styles_DraggableComponent-action__LbbWP"} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We will need to expose a component for this, as this class is unstable
[fieldName]: value, | ||
}; | ||
<div {...{ inert: !isEditable ? "" : undefined }}> | ||
{!isEditable && ( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think it would work better to just turn all fields into readOnly
so the user can still read the fields.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That was my initial approach, but it didn't work on all fields e.g. the External and array on the hero.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If that's the case, I think we should address those as bugs instead
export type OverlayActions = { | ||
isEditable?: boolean; | ||
isDuplicatable?: boolean; | ||
isDeleteable?: boolean; | ||
isDraggable?: boolean; | ||
}; | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we should
- Call this
permissions
- Use verbs instead of adjectives
export type OverlayActions = { | |
isEditable?: boolean; | |
isDuplicatable?: boolean; | |
isDeleteable?: boolean; | |
isDraggable?: boolean; | |
}; | |
export type Permissions = { | |
edit?: boolean; | |
duplicate?: boolean; | |
remove?: boolean; | |
drag?: boolean; | |
[key: string]: boolean; // For custom permissions | |
}; |
Another proposal that differs in two key ways:
"use client";
import { ActionBar, Puck, usePuck } from "@/core";
import { Lock, Unlock } from "lucide-react";
import { useState } from "react";
const data = {};
export function Editor() {
const [permissions] = useState({
duplicate: false,
edit: true,
// A custom permission for the user
party: true,
});
const [myComponentPermissions] = useState({
edit: true,
party: false,
});
const [localPermissions, setLocalPermissions] = useState<
Record<string, object>
>({});
return (
<Puck
// Specify global permissions
permissions={permissions}
// Track permissions in UI state to enable modifying via dispatcher
config={{
components: {
MyComponent: {
// Basic usage
permissions: myComponentPermissions,
// Advanced usage
resolvePermissions: ({
props,
lastPermissions,
initialPermissions,
}) => {
if (localPermissions[props.id]?.edit === false) {
return {
...myComponentPermissions,
edit: false,
};
}
return myComponentPermissions;
},
},
},
}}
data={data}
overrides={{
// Enable override of entire action bar, not just internal actions
actionBar: ({ children, item, itemSelector }) => {
// New API to get resolved permissions
const { getPermission } = usePuck();
const { id } = item.props;
// Get resolved permission
const isEditable = getPermission({ permission: "edit", itemSelector });
return (
// <ActionBar> is the default UI
<ActionBar>
<div
style={{
display: "flex",
}}
>
{isAdmin && (
<ActionBar.Action
onClick={() => {
setLocalPermissions((obj) => ({
...obj,
[id]: { ...obj[id], edit: !isEditable },
}));
}}
>
{!isEditable ? <Unlock size={16} /> : <Lock size={16} />}
</ActionBar.Action>
)}
</div>
{children}
</ActionBar>
);
},
}}
/>
);
} |
Closes #461
Still todo: