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

Remove text and image default blocks #2410

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

Conversation

driftingly
Copy link
Contributor

@driftingly driftingly commented Jan 26, 2024

As someone new to Twill, I was tripped up by the default blocks.
This could be simply user error or education is needed.

The other option would be to update the twill:install command to create the preview/site files for the default blocks.

Steps to reproduce:

  • Create a new laravel app, laravel new twill-test
  • Require Twill composer require area17/twill:"^3.0"
  • Install Twill php artisan twill:install
  • Build frontend npm install && npm run dev
  • Create a basic module php artisan twill:make:module -B pages
  • Update AppServiceProvider to add a navigation link
  • Run migrations php artisan migrate
  • Update PageController.php to add a block editor to the getForm method
$form->add(
    BlockEditor::make()
);  
  • Log into the admin panel
  • Add a new page
  • Open block editor
  • Drag Body text component onto the editor
  • Add text content
  • Get error:
    • An error occurred trying to render:
    • View [site.blocks.text] not found. in [...]/vendor/laravel/framework/src/Illuminate/View/FileViewFinder.php

We must create text.blade.php in resources/views/site/blocks to fix the error.

It took a lot of work to figure out what to put in the blade file to access the content.

I searched Twill for Body text and got a hit on src/Commands/stubs/blocks/text.blade.php. The file includes two translatable blade components for the title and text, which were red herrings. I finally figured out the stub uses the @twillBlockCompiled('true') directive, resulting in the Vue component frontend/js/components/blocks/BlockWysiwyg.vue being used instead. The Vue component defines a single WYSIWYG editor with a name of html.

Contents of text.blade.php

<div>
    {!! $block->wysiwyg('html') !!}
</div>

Or

<div>
    {!! $block->input('html') !!}
</div>

Screenshot 2024-02-02 at 3 40 33 PM Screenshot 2024-02-02 at 3 40 44 PM

@ifox
Copy link
Member

ifox commented Feb 1, 2024

Thanks for the amazing report in this PR @driftingly.

I'm totally with you on this. This is definitely a mix of lacking documentation on these default blocks and legacy code for how all blocks used to be defined in earlier versions of Twill. Creating new blocks back then required generating Vue SFCs from the blocks' Blade form files followed by a build. Compiled blocks are still valuable for custom needs, but as you probably figured, none of that complexity is necessary anymore.

These default blocks aren't even meant to be used, since Twill is really about giving developers control of creating their own blocks, instead of being opinionated about it. Based on that, I was initially in favor of merging your PR. But thinking about it more, something doesn't feel right if we do, because instead of an error due to a missing frontend rendering file (which is definitely where Twill doesn't want to be opinionated at all), we'd get a weirdly empty block editor field when adding the field to a form before creating any blocks.

So I'm split between accepting this or refactoring and documenting it properly instead.

What was your expectation going in? Did you appreciate that there were 2 generic blocks provided by default until you had trouble using them? Or did you/do you feel like they shouldn't be there in the first place?

I like your idea of generating the preview files. They could be super basic html but are likely to be helpful for new comers. It just feels a bit invasive to publish those files from the get go, especially in the current default site folder, which might already contain views in existing Laravel apps. I know it's very much an edge case, but I'm trying to look at this from every angle. This could be addressed in a future major version by publishing to a twill folder.

Let me know what you think. For now I'm still inclined to merge this and revisit in the future if we feel like having some default blocks is preferred.

@driftingly
Copy link
Contributor Author

driftingly commented Feb 2, 2024

@ifox, I agree with your hesitation. Seems like without publishing some starting point to the front end, neither option is ideal.

The documentation I've seen on the Twill site around blocks usually starts by removing the default ones, so I'm not sure there is value in keeping them.

If we wanted to move forward with this PR I tried to improve the messaging around the initial empty block state. Let me know how I could improve this, or if you could point me to somewhere in the app where something like this is already done.

Possibly link out to the documentation?

301995991-484237bc-95d0-4366-b39b-d9bae8866673 301996009-a42a1944-dbe9-4eb6-95af-955cdb41edc9

lang/en/lang.php Outdated Show resolved Hide resolved
@Tofandel
Copy link
Contributor

Maybe we should also update the content of the stubs\blocks\text.blade.php which is completely different than the actual compiled component which are actually not translatable, don't have a title input and a different name

Currently

@twillBlockCompiled('true')
@twillBlockComponent('a17-block-wysiwyg')
@twillBlockTitle('Body text')
@twillBlockIcon('text')
@twillBlockGroup('twill')

<x-twill::input
    name="title"
    label="Title"
    :translated="true"
/>

<x-twill::wysiwyg
    name="text"
    label="Text"
    placeholder="Text"
    :toolbar-options="[
        'bold',
        'italic',
        ['list' => 'bullet'],
        ['list' => 'ordered'],
        [ 'script' => 'super' ],
        [ 'script' => 'sub' ],
        'link',
        'clean'
    ]"
    :translated="true"
/>

But it should be more like

@twillBlockCompiled('true')
@twillBlockComponent('a17-block-wysiwyg')
@twillBlockTitle('Body text')
@twillBlockIcon('text')
@twillBlockGroup('twill')

<x-twill::wysiwyg
    name="html"
    label="Text"
    placeholder="Text"
    :toolbar-options="[
        'bold',
        'italic',
        ['list' => 'bullet'],
        ['list' => 'ordered'],
        [ 'script' => 'super' ],
        [ 'script' => 'sub' ],
        'link',
        'clean'
    ]"
    :translated="false"
/>

Or remove the content all together to not mislead in believing the content is coming from them

@Tofandel
Copy link
Contributor

Tofandel commented Feb 26, 2024

Also related issue, even though I disabled all twill default blocks in the config, the block templates are still present on the page (Eg: Carousel, Gallery)

image

Is this normal/desired?

@ifox
Copy link
Member

ifox commented Mar 1, 2024

@Tofandel it is normal considering the code, but not really desired indeed as it bloats the page even though they are not needed (not only default twill blocks, but all blocks that haven't been allowed on a specific form).

@ifox ifox added this to the Twill 4.0 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.

3 participants