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

Feature: allow required blocks to move between sections #6972

Merged
merged 10 commits into from
Sep 29, 2023

Conversation

JasonTheAdams
Copy link
Contributor

@JasonTheAdams JasonTheAdams commented Sep 27, 2023

Description

Forms have a number of blocks/fields that are required for the form to function — e.g. donation amount and email. The primary way we've enforced this is to put a lock on the required blocks. This has a couple of flaws:

  1. Locked blocks can't be moved out of their parent block, meaning moving these blocks between sections wasn't possible
  2. The user can unlock blocks anyway, allowing them to delete the block... making this a moot point

We also have a validation system on the server which checks that the required blocks are present when saving. It's simply not likely that folks will remove things like the name and amount field, which means we're creating friction to do something relatively likely (move blocks between sections) for something unlikely that's prevented another way.

This PR removes the locks on the required fields, allowing them to move freely. It also updates the server error flow to display a Gutenberg model instead of an alert, giving a nicer experience.

Affects

The required blocks and the block validation system.

Visuals

image

Testing Instructions

Move the required blocks around the form builder. Then delete one and try to save. Finally, add it back and attempt to save, making sure it can recover from an error state.

Pre-review Checklist

  • Acceptance criteria satisfied and marked in related issue
  • Reviewed by the designer (if follows a design)
  • Self Review of code and UX completed

@JasonTheAdams
Copy link
Contributor Author

@jonwaldstein This is ready for review!

@jdghinson
Copy link
Contributor

jdghinson commented Sep 27, 2023

@JasonTheAdams Looks good. Let's add the error icon to the header text like the image below to emphasize on it being an error.

Suggestion: Can we also have a cta like "add missing blocks" instead of "Got it" which automatically adds the block to their original/intended position?

Screenshot 2023-09-27 at 8 12 46 AM

@jonwaldstein
Copy link
Contributor

@JasonTheAdams i'm getting a bunch of typescript errors on this PR when running npm run dev. is this happening for you?

Screenshot 2023-09-27 at 1 17 23 PM

@JasonTheAdams
Copy link
Contributor Author

JasonTheAdams commented Sep 28, 2023

@JasonTheAdams Looks good. Let's add the error icon to the header text like the image below to emphasize on it being an error.

All set!
image

Suggestion: Can we also have a cta like "add missing blocks" instead of "Got it" which automatically adds the block to their original/intended position?

I like the idea of this, but I think it's a bit beyond the scope of this right now. This is also an unlikely scenario. We can improve this later.

@JasonTheAdams
Copy link
Contributor Author

@JasonTheAdams i'm getting a bunch of typescript errors on this PR when running npm run dev. is this happening for you?

Screenshot 2023-09-27 at 1 17 23 PM

Yikes, yeah, you're right @jonwaldstein. None of these have anything to do with what I'm doing, though. Must be from a previous PR. It's weird that these only show up when you run npm run dev and not npm run watc:v3. 😕

@jonwaldstein
Copy link
Contributor

@JasonTheAdams i'm getting a bunch of typescript errors on this PR when running npm run dev. is this happening for you?
Screenshot 2023-09-27 at 1 17 23 PM

Yikes, yeah, you're right @jonwaldstein. None of these have anything to do with what I'm doing, though. Must be from a previous PR. It's weird that these only show up when you run npm run dev and not npm run watc:v3. 😕

yeah, it's something I need to look into but basically laravel mix will prevent bundle from typescript errors while wp scripts ignores them.

Copy link
Contributor

@jdghinson jdghinson left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good

@JasonTheAdams
Copy link
Contributor Author

Any other feedback, @jonwaldstein, besides the console errors unrelated to this PR?

@jonwaldstein
Copy link
Contributor

Any other feedback, @jonwaldstein, besides the console errors unrelated to this PR?

@JasonTheAdams i'm only getting these build errors on this PR - not develop so that should be resolved before merging

Copy link
Member

@rickalday rickalday left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Passed manual QA tests.

@JasonTheAdams JasonTheAdams merged commit c9b8d73 into develop Sep 29, 2023
20 of 21 checks passed
@JasonTheAdams JasonTheAdams deleted the feature/drag-blocks-between-sections branch September 29, 2023 17:46
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.

4 participants