-
Notifications
You must be signed in to change notification settings - Fork 191
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
Feature: Preview API #7059
Feature: Preview API #7059
Conversation
Just looking at the gif, @alaca, this looks amazing! Can we remove the debouncing on the text fields so the typing is instantaneous? 😃 |
@JasonTheAdams sure 😃 |
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.
@alaca this is amazing! Just had some thoughts and suggestions. We'll also need to circle back and make sure all settings are accountable. One I noticed we still need to figure out is the custom css setting.
src/FormBuilder/resources/js/form-builder/src/settings/donation-goal/index.tsx
Outdated
Show resolved
Hide resolved
src/FormBuilder/resources/js/form-builder/src/settings/donation-goal/index.tsx
Outdated
Show resolved
Hide resolved
src/FormBuilder/resources/js/form-builder/src/settings/donation-goal/index.tsx
Outdated
Show resolved
Hide resolved
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'm so happy about this 😭, great job @alaca!
…alues are correct
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.
@jonwaldstein @alaca What I believe is my last question: Why are we prop-drilling the form data instead of using a context?
@JasonTheAdams that's because the current state is divided and we use two state providers In other words, it was easier to do it this way 😅 |
@alaca Hahah! Got it. I'm just a bit concerned that it's not easily available, especially for when we get into form design settings, that could apply anywhere in the node tree. Seems like we need to make sure, one way or the other, it's globally available without having to prop-drill. Seem sensible to you, @jonwaldstein? |
@JasonTheAdams i'm open to adding the form data object into context, but as @alaca pointed out - it's not that straight forward. This is also specific to the preview api as window data still works as expected for live rendering. My preference would be to keep the prop drilling for now (since it's really just for the header) and update the context structure (as needed) when we actually introduce a form design api that requires it. At that time, I would explore breaking out that form data object into pieces that make more sense for the front-end application which could end up being separate context providers. |
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'm SO EXCITED for this!! 🤩
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.
Passed manual tests.
Description
This PR introduces a new Preview API that is used to pass the state changes in design mode to the form inside the iframe. Preview API is a simple pub/sub system that hooks up in the form state which makes all changes in design mode show up instantly on the screen without reloading the form.
Affects
Form Builder
Donation form
Visuals
Testing Instructions
Pre-review Checklist
@unreleased
tags included in DocBlocksPlugin zip for QA: https://github.com/impress-org/givewp/suites/17790212597/artifacts/1019602715