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/234: Add Required Component #235

Closed
wants to merge 9 commits into from
Closed

feat/234: Add Required Component #235

wants to merge 9 commits into from

Conversation

Sidsector9
Copy link
Member

@Sidsector9 Sidsector9 commented Jun 29, 2023

Description of the Change

Video

Screen-2023-07-02-223149.mp4

Closes #234

How to test the Change

Changelog Entry

Added - Required component

Credits

Props @fabiankaegy @Sidsector9

Checklist:

  • I agree to follow this project's Code of Conduct.
  • I have updated the documentation accordingly.
  • I have added tests to cover my change.
  • All new and existing tests pass.

@github-actions
Copy link

github-actions bot commented Jun 29, 2023

Size Change: +2.19 kB (+4%)

Total Size: 55.1 kB

Filename Size Change
dist/index.js 55.1 kB +2.19 kB (+4%)

compressed-size-action

@Sidsector9 Sidsector9 force-pushed the feat/234 branch 2 times, most recently from 40668bc to 57f2510 Compare June 29, 2023 13:23
@Sidsector9 Sidsector9 changed the title WIP: feat/234: Add 'Required` Component WIP: feat/234: Add Required Component Jun 29, 2023
@Sidsector9
Copy link
Member Author

@fabiankaegy I took a slightly different approach to show the error messages. (Video added in the PR description)

I created a custom data store that keeps track of all the Blocks that uses the Required component and adds an inline error message under the block if the required field(s) are not set.

Wanted to check with you if you have a better approach before I continue with writing the tests.

@Sidsector9 Sidsector9 changed the title WIP: feat/234: Add Required Component feat/234: Add Required Component Jul 4, 2023
@fabiankaegy
Copy link
Member

Hey @Sidsector9 👋

This is super cool :)

Can you share a little more detail about why you decided to go with this approach? Was it because the other approach didn't work or because the messaging for users was unclear/confusing?

Would love to better understand the constraints here :)

@Sidsector9
Copy link
Member Author

Sidsector9 commented Jul 4, 2023

Sure @fabiankaegy.

Was it because the other approach didn't work?

I was able to lock/unlock the post by providing unique IDs to a Required component's lockPostSaving() and unlockPostSaving() functions.
So even if a post contains multiple blocks, each block with multiple Required components, the post will remain locked until all Required components trigger unlockPostSaving() with their unique ID. It worked so far.

Why you decided to go with this approach?

  1. I couldn't find a way to send the information about the block containing the Required component to the Pre publish panel for displaying the notice without a custom data store.
  2. The Pre publishing panel is only available for a new unpublished post. Once it is published, the "Update" button is disabled out. I thought of adding the notices to the PostStatus panel but didn't go through with it to avoid clutter.
  3. I thought it would be cool to highlight the block as soon as it fails validation, and not wait for the editor to reach the Prepublish panel. This way, we avoid the implementation of showing the warning/error in the Prepublish panel and Notices.
  4. This also works well when the input fields of a Block are hidden by default and only rendered when the Block is selected.

I would love to hear if you have a better approach, somewhere I strongly believe you do 😄

@fabiankaegy
Copy link
Member

@Sidsector9 Thank you again for working on this! It does exactly what I was looking and hoping for. I'm going to close this PR anyways for now, as @darrenjacoby came up with a really cool more powerful way to solve the same issue in #238

I still like the cleanliness of the Required component. But having the ability for custom validations is really powerful and at the same time also solves all the things we were tying to create here.

@fabiankaegy fabiankaegy mentioned this pull request Jul 28, 2023
4 tasks
@Sidsector9
Copy link
Member Author

I just saw @darrenjacoby's PR and I agree, it does a lot of other validations that this PR doesn't. Completely on board with the decision 👍

@fabiankaegy fabiankaegy deleted the feat/234 branch February 11, 2024 21:57
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.

Create a new Required component
2 participants