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

Move svg icons to Blade partials #990

Merged
merged 7 commits into from
Jun 24, 2021

Conversation

antonioribeiro
Copy link
Member

Problem

While trying to deploy a Twill app on Laravel Vapor we found an issue related to the icons, which are SVG stuffed directly into the HTML, at Blade compile time:

@if(config('twill.dev_mode', false))
    {!! file_get_contents(twillAsset('icons.svg')) !!}
    {!! file_get_contents(twillAsset('icons-files.svg')) !!}
    {!! file_get_contents(twillAsset('icons-wysiwyg.svg')) !!}
@else
    {!! File::exists(public_path(twillAsset('icons.svg'))) ? File::get(public_path(twillAsset('icons.svg'))) : '' !!}
    {!! File::exists(public_path(twillAsset('icons-files.svg'))) ? File::get(public_path(twillAsset('icons-files.svg'))) : '' !!}
    {!! File::exists(public_path(twillAsset('icons-wysiwyg.svg'))) ? File::get(public_path(twillAsset('icons-wysiwyg.svg'))) : '' !!}
@endif

The problem is that Vapor deletes all files from the public directory (they are uploaded to S3 during deployment) and we don't see a way of telling it to not do it unless we fork the vapor-cli script and change it ourselves.

The current implementation of Twill expects the SVG files to be present on /public. Even if we overload twillAsset() to load those files from a different directory (or on S3), Twill is still expecting them to be inside /public, as per the snippet above.

Description

This PR moves all .svg icon files to the folder views/partials/icons and import them as normal blade templates:

<div class="svg-sprite">
    @include('twill::partials.icons.icons-svg')

    @include('twill::partials.icons.icons-files-svg')

    @include('twill::partials.icons.icons-wysiwyg-svg')
</div>

I removed the creation of hashed (versioned) files, because blade files will be recompiled automatically by Laravel when they change.

@antonioribeiro
Copy link
Member Author

antonioribeiro commented Jun 23, 2021

@ifox , can you, please, do a quick review? No need to merge it now, just to check the solution, because we are probably using this (as a fork) on a project on Vapor today, but if this implementation is not approved, we may be stuck with the fork forever, or we will have to get back to this to use a new implementation or do something different to get back to Twill 2.x.

@antonioribeiro antonioribeiro requested a review from ifox June 23, 2021 10:50
@ifox
Copy link
Member

ifox commented Jun 23, 2021

This solution looks good to me and I will approve and merge as soon as I can make sure this has no impact elsewhere.

@antonioribeiro
Copy link
Member Author

Awesome, thanks @ifox !!

@ifox
Copy link
Member

ifox commented Jun 24, 2021

/build-frontend

@ifox
Copy link
Member

ifox commented Jun 24, 2021

/build-frontend

@ifox
Copy link
Member

ifox commented Jun 24, 2021

/build-frontend

@ifox ifox merged commit 3b386d7 into area17:2.x Jun 24, 2021
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