-
Notifications
You must be signed in to change notification settings - Fork 87
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(v2/end-page-builder-2) edit end page from builder #4047
feat(v2/end-page-builder-2) edit end page from builder #4047
Conversation
|
||
interface BuilderDrawerContainerProps { | ||
title: string | ||
content: JSX.Element |
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.
nit: probably can use react's children
prop instead, then we can just inline the content instead of assigning it to the `contenq prop
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.
Got it, updated to use children
prop
Yet to cleanup the TODO comments
9e6407b
to
191f7ef
Compare
{stateData.state === BuildFieldState.EditingEndPage ? ( | ||
<EndPageView /> | ||
) : ( | ||
<FormBuilder placeholderProps={placeholderProps} /> | ||
)} |
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.
instead of rendering between end page and builder view, should we just render both and hide them appropriately (by toggling display with display
css prop)? This allows for the state and scroll position of either previews to be maintained
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.
Good idea, I've updated the PR to do that, thanks
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.
lgtm, please request review from pearly in chromatic next time!
> | ||
<Stack w="100%"> | ||
<Flex justifyContent="center" pt="1rem" pb="0.5rem"> | ||
<Image src={form?.admin?.agency?.logo ?? ''} h="4rem" /> |
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.
nit: do we need the empty string ''
default? src
accepts undefined
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.
Oh then we don't need ''
default! Removed it
Builds on #3983
Problem
Second and last part of end page builder PR.
This PR adds the functionality to build the end/thank you page through the form builder
Closes #3361
Solution
Breaking Changes
Before & After Screenshots
Stories for
AdminFormPage
/Create
will reflect the change