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.

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(editor) include query parameters as editor params #60

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

maarten2424
Copy link
Contributor

@maarten2424 maarten2424 commented Jun 17, 2024

Description

Currently, the following query parameters are only mutable on the URL.

The editor page takes a few important query parameters:

readOnly <boolean> = true - if set to true, no permanent modifications will be done to existing documents or templates. Very good way to play around, debug components, documents, etc. When you open the editor page directly it's by default set to true in order to prevent unexpected modifications.

document <string> - the id of the document you want to open. Leaving it undefined means you're creating a new document.

rootComponent <string> - the id of the root component. Mandatory when creating a new document (document param is not set).

rootTemplate <string> - the id of the template to copy when creating a new document. Can't be specified together with rootComponent.

locale <string> - a locale id. The locale must exist in Config.locales.

This MR enables the parameters to be set as parameters on the editor itself, making them mutable and react when changed. This way changes to any of these parameters won't result in a refresh of the editor, and making it more usable in dynamic hosted environments.

Copy link

vercel bot commented Jun 17, 2024

@maarten2424 is attempting to deploy a commit to the Shopstory Team on Vercel.

A member of the Team first needs to authorize it.

@maarten2424 maarten2424 force-pushed the feat/query-parameters-as-editor-options branch 2 times, most recently from d86e458 to e5e316f Compare June 18, 2024 11:56
@r00dY
Copy link
Contributor

r00dY commented Jul 9, 2024

@maarten2424 So the assumption is that you can re-render the editor based on input params, right? It makes sense, although I'm not sure it will work right now with the changes in the PR. There are some useEffect with [] dep array or memo in the initial render phase of the editor. Unless those are addressed I don't think it's gonna work. How was it tested?

@maarten2424 maarten2424 force-pushed the feat/query-parameters-as-editor-options branch from e5e316f to 4d4a81c Compare July 10, 2024 15:57
@maarten2424
Copy link
Contributor Author

Exactly, changing those will re-render easyblocks. There where some initial issues with the memo and document ID's for the form, but it seems to work properly now. I've tested changing all the properties directly and the edit seems to re-render quickly!

@r00dY
Copy link
Contributor

r00dY commented Jul 23, 2024

  1. Shouldn’t we show some loader when Editor is being reloaded?
  2. Shouldn’t we force-save old content when reload happens?
  3. Have you tested the history stack between reloads (should be nulled)?

Loose thought: what if we just unmount the root editor, show loader and mount the editor root component again? It could also work without loader. Wouldn't it meet your criteria? It would be much more bulletproof as there would be no state dependence between old and new instances. And there's quite a lot of state over there...

@maarten2424
Copy link
Contributor Author

  1. Shouldn’t we show some loader when Editor is being reloaded?
  2. Shouldn’t we force-save old content when reload happens?
  3. Have you tested the history stack between reloads (should be nulled)?

Loose thought: what if we just unmount the root editor, show loader and mount the editor root component again? It could also work without loader. Wouldn't it meet your criteria? It would be much more bulletproof as there would be no state dependence between old and new instances. And there's quite a lot of state over there...

Good point on the state management. I've been trying to overcome the loading and do instant state change, but I think initialisation time would be minimal. Let me prototype this idea in this PR and come back to you!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants