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

Refactor and improve list of available blocks #2513

Open
wants to merge 20 commits into
base: 3.x
Choose a base branch
from

Conversation

Tofandel
Copy link
Contributor

@Tofandel Tofandel commented Mar 1, 2024

Description

This PR adresses issues regarding performance of having a lot of defined blocks combined with a lot of block editors on a page

It also improves the way we can exclude blocks with a global exclusion list accepting names of blocks as well as allowlist by group and callbacks running on block, allowing for more advanced filter using component methods or regexes for example

It also adds the ability to sort blocks (before they could only be sorted by the order in the directory or in the block list which resulted in a messy list of blocks)

excludeBlocks now also accepts a callback as an argument

It also now only prints templates of all the availableBlocks on the page (it still prints all the repeaters templates and not just the used ones though)

TODO

  • Update documentation
  • Allow callback in the allow block as well
  • Only display list of all used blocks on the page and not all of them

});

// @TODO: Can be replaced with doesntContain in twill 3.x
if ($hasRequiredRule && !in_array($fieldRules, $fieldRules->toArray())) {
Copy link
Contributor Author

@Tofandel Tofandel Mar 1, 2024

Choose a reason for hiding this comment

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

This is meant for a different PR, but if you're okay with it we can leave it in direclty as well, I was just chasing bugs and I stumbled accross
!in_array($fieldRules, $fieldRules->toArray()) which is very weird and also always true

Copy link
Member

Choose a reason for hiding this comment

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

This used to be a check against 'nullable', see 315082b#diff-0408ed612a5dd9175dea4ee82126a50940e6809907eae490aa1abf94e10a9986R94 and it looks like it was wrongly replaced during another change.

I'm fine with keeping this in this PR.

@ifox
Copy link
Member

ifox commented Mar 1, 2024

Great stuff, thank you!

I'm not sure I'm following your point about the ability to sort blocks. The order specified in the array of available blocks passed to the block editor field is already preserved, so I'm curious what you mean.

@Tofandel
Copy link
Contributor Author

Tofandel commented Mar 1, 2024

Ah yes I see what the sortBy was for then, I'll make sure to restore it if a block list is given

The issue is now the rules have gotten a lot more options so it's especially more likely a list of blocks is not given and only working from excluded blocks or a list of groups in which case we can't order by the list of blocks so we need more sorting options

Maybe we can add a config list of block order, only used for sorting

@Tofandel Tofandel force-pushed the feat/exclude-callback branch 2 times, most recently from 21bb5d5 to c33edd7 Compare March 5, 2024 17:15
@Tofandel Tofandel force-pushed the feat/exclude-callback branch 2 times, most recently from 8c738d4 to 33b059f Compare March 11, 2024 21:45
@Tofandel
Copy link
Contributor Author

I didn't yet figure out what's causing the js error in the tests as when using the PR anywhere else there is no issue, I can't run dusk on WSL to figure it out

This PR would take care of
https://github.com/orgs/area17/projects/3/views/6?pane=issue&itemId=12754007

@Tofandel
Copy link
Contributor Author

Tofandel commented May 24, 2024

I need to check why the tests are failing, only the Web tests are failing, it seems due to a js error, how can I debug this (in a normal instance I never encountered any) ?

@ifox
Copy link
Member

ifox commented May 24, 2024

If you check the artefact at the bottom of https://github.com/area17/twill/actions/runs/9214252760, you'll see this error:

failure-A17_Twill_Tests_Browser_BlockEditorMediaTest_testMediaCropsForNewBlocks-0.png

@Tofandel
Copy link
Contributor Author

Ah thanks that's very helpful, so weird though that I'm not getting any error myself when it's in the main.blade.php template

@Tofandel
Copy link
Contributor Author

Okay I know why, it's because TwillBlocks is registered in the aliases in composer.json, but aliases are not applied when running the tests locally

});

// @TODO: Can be replaced with doesntContain in twill 3.x
if ($hasRequiredRule && !in_array($fieldRules, $fieldRules->toArray())) {
Copy link
Member

Choose a reason for hiding this comment

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

This used to be a check against 'nullable', see 315082b#diff-0408ed612a5dd9175dea4ee82126a50940e6809907eae490aa1abf94e10a9986R94 and it looks like it was wrongly replaced during another change.

I'm fine with keeping this in this PR.

@ifox
Copy link
Member

ifox commented May 28, 2024

This PR would take care of https://github.com/orgs/area17/projects/3/views/6?pane=issue&itemId=12754007

That roadmap item isn't about disabling blocks programmatically, but that's a good addition, still! Disabled blocks in the roadmap are about a feature in the CMS UI to let users disable a specific block (so they can keep the content and not render it on the frontend). This can already be achieved easily by adding a field in every block, but we'd like to support it better natively.

@Tofandel
Copy link
Contributor Author

@ifox I see, yes it would be nice, much like a "Draft" feature on a block basis

@ifox ifox added this to the Twill 3.4 milestone Jun 4, 2024
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.

2 participants