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: GiveWP BlockTypes #7195

Merged
merged 16 commits into from Apr 12, 2024
Merged

Feature: GiveWP BlockTypes #7195

merged 16 commits into from Apr 12, 2024

Conversation

jonwaldstein
Copy link
Contributor

@jonwaldstein jonwaldstein commented Jan 19, 2024

Description

Introducing GiveWP BlockTypes!

The idea for GiveWP BlockTypes is to take generic block models and convert them into identifiable first-class BlockType models that have type-enabled properties and built-in casting.

Instead of this:

case "givewp/text":
  return Text::make(
      $block->hasAttribute('fieldName') ?
          $block->getAttribute('fieldName') :
          $block->getShortName() . '-' . $blockIndex
  )->storeAsDonorMeta(
      $block->hasAttribute('storeAsDonorMeta') ? $block->getAttribute('storeAsDonorMeta') : false
  )->description($block->getAttribute('description'))
      ->defaultValue($block->getAttribute('defaultValue'))

We would have this:

case TextBlockType::name():
  $textBlockType = new \Give\FormBuilder\BlockTypes\TextBlockType($block);

  return Text::make(
      $textBlockType->fieldName ?? $block->getShortName() . '-' . $blockIndex
  )->storeAsDonorMeta($textBlockType->storeAsDonorMeta)
      ->description($textBlockType->description)
      ->defaultValue($textBlockType->defaultValue);

Thoughts:

  • Would be nice to bridge the server block types and client blocks with a block.json spec. Maybe the $properties array actually gets generated by the block.json file one day 🤯
  • Could explore more deep integration of these specific block types into our conversion process and/or programmatic block creation
  • Integrating Block Types into the migration process would potentially be less tedious & more predictable.
  • Sharing common attributes like label, fieldName, conditionalLogic, etc using traits.
  • Adding methods like $blockType->toField() would make the block to field conversion cleaner and more testable.

Affects

Currently this is not being used anywhere.

Visuals

N/A

Testing Instructions

This would need to be done programatically

Pre-review Checklist

  • Acceptance criteria satisfied and marked in related issue
  • Relevant @unreleased tags included in DocBlocks
  • Includes unit tests
  • Reviewed by the designer (if follows a design)
  • Self Review of code and UX completed

@jonwaldstein jonwaldstein marked this pull request as ready for review January 30, 2024 14:42
@jonwaldstein jonwaldstein changed the title Fun: GiveWP BlockTypes Feature: GiveWP BlockTypes Jan 30, 2024
@jonwaldstein
Copy link
Contributor Author

@kjohnson i've been gradually working on this during my Fridays. would love to get your thoughts on it.

Copy link
Member

@kjohnson kjohnson left a comment

Choose a reason for hiding this comment

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

I like this idea. It would certainly help to make the conversion process more easily testable and could be beneficial to mirror the typing on the server side.

Copy link

This PR is stale because it has been open 45 days with no activity. Stale PRs will NOT be automatically closed.

@github-actions github-actions bot added the Stale label Mar 26, 2024
@jonwaldstein
Copy link
Contributor Author

@kjohnson anymore thoughts on this? If not, I would like to merge and start using it 🧙

@kjohnson
Copy link
Member

anymore thoughts on this? If not, I would like to merge and start using it 🧙

No objections here. I gave it another look over and it looks good to me.

Having tests here is a big help!

@jonwaldstein jonwaldstein merged commit 2f2310d into develop Apr 12, 2024
20 checks passed
@jonwaldstein jonwaldstein deleted the fun/givewp-block-types branch April 12, 2024 21:52
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.

None yet

2 participants