-
Notifications
You must be signed in to change notification settings - Fork 338
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
Functionality for switching between editing and interactive mode #674
base: main
Are you sure you want to change the base?
Conversation
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
@SicParv1sMagna 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.
Nice one, I really like the idea of keeping this in state! I've left some comments.
My only concern I have is that this is going to clash with #556, which rebuilds DraggableComponent.
If you address my comments, we can figure out the conflicts before the next feature release, which will probably include #556.
}; | ||
}, | ||
{} | ||
) |
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 your local prettier is conflicting with the shared prettier. Can you run npm run format
and commit the results?
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.
done
<div className={getClassName("overlay")} style={{ ...(isInteractive && { cursor: 'default', background: 'transparent' }) }} /> | ||
<div className={getClassName("contents")} style={{ ...(isInteractive && { pointerEvents: 'auto' }) }}>{children}</div> |
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.
These would probably be better as a CSS class, passed to the getClassName
function here.
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.
done
@@ -72,6 +72,8 @@ export type AppContext< | |||
plugins: Plugin[]; | |||
overrides: Partial<Overrides>; | |||
history: Partial<PuckHistory>; | |||
isInteractive: 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.
Any ideas for how you could add this to the custom-ui
demo? I like to use features in demos for easier reviewing, and so we can be sure things build properly.
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.
Is there any information about future release? How soon can I start using this changes?
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.
done
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've been doing some thinking and this will actually be better on the AppState rather than a separate status. This saves us polluting the usePuck hook with every piece of appState, and introducing a custom dispatcher (setIsInteractive
)
I also think that isInteractive
is probably not quite the right name. I would prefer to introduce a new parameter to AppState, either mode
or previewMode: "interactive" | "edit"
. Does that make sense?
Having said that, I know you're keen to get this done, so I'm happy to do a canary containing this existing implementation if it would help you out.
I think that you're right, but as I understand, usePuck is allowed either on "out of box" solution (default ), either on custom layout side, so I would like to use it from usePuck hook, cus I'am using custom layout. @chrisvxd what do you think about this? |
@SicParv1sMagna the app state is exposed via usePuck, and can be controlled via the dispatcher: https://puckeditor.com/docs/api-reference/functions/use-puck#appstate I'm not convinced on the naming yet. For now, let's just move |
@chrisvxd Hello, sorry for fixing this pr so long. Removed isInteractive from usePuck hook and renamed it to mode |
@chrisvxd is my merge request still actual? |
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.
Hey @SicParv1sMagna! Sorry for the delay.
Just reviewing now and this doesn't quite behave as expected. When I switch to interactive mode, the items are still draggable and action bar overlay shows. I think it might be better to modify DropZone to avoid rendering altogether when in interactive
mode.
@chrisvxd Missed that moment, fixed it, check it please) |
@chrisvxd Can you please check new version |
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.
Hi @SicParv1sMagna - I've addressed merge conflicts, but you still need to update your tests for this to pass CI. Please run cd packages/core && npm run test -u
and commit the updated snapshot change.
Give me few minutes |
Done |
Recently I was trying to add PR for switching between editing and interactive mode PR. So, after a small conversation with Chris Villa in discord thread, he said that you want to add toggle in header for switching modes, I have added new functionality in context, which allows user to interact with components inside preview in "out of box" editor and in custom editor.
Closes issue #565