impress-org / givewp-next-gen

GiveWP 3.0.0 Feature Plugin
https://givewp.com/
GNU General Public License v3.0
10 stars 1 forks source link

Feature: add server-side required block validation to form builder #228

Closed jonwaldstein closed 1 year ago

jonwaldstein commented 1 year ago

Description

There is a short list of required blocks that are necessary to build a donation form. This updates the form builder to add validation for those required blocks, when updating a form.

Previously we were trying to validate field names from the fields api conversion - which in practice had no context for the admin. This is much more clear what's going on so the admin can simply add the missing required block if it is not in the form for some reason.

Affects

The form builder updating functionality.

Visuals

Screenshot 2023-07-25 at 1 24 11 PM

Testing Instructions

Pre-review Checklist


JasonTheAdams commented 1 year ago

@jdghinson Do you think a generic browser alert works here? Should we use a modal or something else?

jdghinson commented 1 year ago

@jdghinson Do you think a generic browser alert works here? Should we use a modal or something else?

@JasonTheAdams Yeah a modal will work but also can we have a confirmation modal when a user tries to remove a block that is required to build a donation form and have this last check as a safety net when they end up deleting, which I'm sure our first confirmation modal would prevent anyway.

jonwaldstein commented 1 year ago

@JasonTheAdams @jdghinson this is intended to be more of a server level catch, if we get to this point (the front-end should be doing it's best to prevent) we just need the user to be able to take action and continue using the form builder (it's currently saying something weird and potentially breaking the form).

While I do agree we can improve the UX by building an error modal, I think it's pretty low on our list of improvements and is something we should circle back to since it would need to be implemented for all form submission errors - and requires some thought into how the application will understand form level errors and where to display them.

I like the idea of adding pro-active validation and would be open to exploring some kind of listener on the front-end to prevent certain blocks from being removed- but would really rather use the native block locking (it's supposed to be a native Gutenberg solution for preventing blocks from being removed) - unfortunately you can still remove an unlocked parent block so it doesn't exactly work the way we want.

In conclusion there are 3 topics here that we can split up into separate pieces:

  1. Server level block validation (this PR)
  2. Front-end block validation / block locking
  3. Form error UI
JasonTheAdams commented 1 year ago

@jonwaldstein I'm totally with you on being careful scope. My only thought was, "hey, I think we have a modal component that would look nicer here!" That was it. If that's not true, we can move along for now.

jonwaldstein commented 1 year ago

@JasonTheAdams got it, at the moment - we don't really have a generic modal component thats styled and to implement here would require a little bit of work since the request is happening outside the block editor. So yeah I would say let's move on for now and can circle back when we want to update the Form errors UI

jonwaldstein commented 1 year ago

@glaubersilva this is ready for review.