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

Add header metas publish command #5552

Merged
merged 4 commits into from
Jul 15, 2024
Merged

Conversation

pxpm
Copy link
Contributor

@pxpm pxpm commented Jul 6, 2024

WHY

BEFORE - What was wrong? What was happening before this PR?

It started with the idea of adding the capability to a Backpack panel to load on mobile devices as a "native app" when bookmarked on homepage in #3318

While doing some information gathering to take the decision to do something about that PR, I found that by default Backpack does not even has a simple favicon meta 😞

That needed to change 👍

AFTER - What is happening after this PR?

Developers can now publish a header_metas file along with some Backpack images as placeholders so that their apps have a favicon, display as an app in mobile, have colored navigation bar when displayed.

image
image
image

HOW

How did you achieve that, in technical terms?

Added a command: backpack:publish-header-metas that will ask some questions to the developer, replace the stubs with the provided values and publish them.

image

Is it a breaking change?

NO, this needs Laravel-Backpack/theme-tabler#185

pxpm and others added 2 commits July 6, 2024 10:25
Copy link
Member

@tabacitu tabacitu left a comment

Choose a reason for hiding this comment

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

Tested, works as expected on local!

I like this a lot, I like how it's not actually anything to maintain, we just help them generate some useful code and files. Good job Pedro!

My only real gripe is with the absolute paths (see my comments) but I've taken the time to scrutinize everything, so I don't have to go through it again 😀

Take a look at my comments please and if you agree it's feasible... run with it!

src/app/Console/Commands/PublishHeaderMetas.php Outdated Show resolved Hide resolved
src/app/Console/Commands/PublishHeaderMetas.php Outdated Show resolved Hide resolved
src/app/Console/Commands/PublishHeaderMetas.php Outdated Show resolved Hide resolved
src/app/Console/Commands/PublishHeaderMetas.php Outdated Show resolved Hide resolved
src/resources/stubs/manifest.stub Outdated Show resolved Hide resolved
src/app/Console/Commands/PublishHeaderMetas.php Outdated Show resolved Hide resolved
src/resources/stubs/header_metas.stub Show resolved Hide resolved
src/app/Console/Commands/PublishHeaderMetas.php Outdated Show resolved Hide resolved
@tabacitu tabacitu removed their assignment Jul 10, 2024
@pxpm pxpm merged commit ec78467 into main Jul 15, 2024
3 checks passed
@pxpm pxpm deleted the save-to-mobile-without-address-bar branch July 15, 2024 07:57
@dimer47
Copy link
Contributor

dimer47 commented Aug 5, 2024

This is a fantastic feature @pxpm ! But there is a slight incompatibility between the commands php artisan backpack:publish-header-metas and php artisan backpack:install -q -n.

When using the php artisan backpack:publish-header-metas command, it prompts for the installation location of the assets. However, by default, the php artisan backpack:install -q -n command installs everything in the public/ directory.

The BackpackServiceProvider.php file located in vendor/backpack/crud/src/BackpackServiceProvider.php contains the following code:

public function publishFiles()
{
    ...
    $backpack_public_assets = [__DIR__.'/public' => public_path()];
    ...
}

Should the assets really be deployed by default during the installation? Or wouldn't it be better to prompt for the installation location of the assets during the installation, with the default value set to /public?

May I create a PR to implement this request, or do you have a different perspective on the matter?

@pxpm
Copy link
Contributor Author

pxpm commented Aug 6, 2024

Hey @dimer47 nice catch! Completely missed that. 😞

I think the correct thing to do is removing that $backpack_public_assets from the publish command as there is nothing in that folder that we would like to publish.

If you have time to create the PR, please go ahead, I will merge and tag a new version right way. 👍

@dimer47
Copy link
Contributor

dimer47 commented Aug 16, 2024

Hey @pxpm, sorry for the delay! 😅

I've gone ahead and created the PR as requested. I've removed the $backpack_public_assets from the publishFiles method, so no assets will be published during the backpack:install command anymore. This ensures that asset deployment is only triggered by the backpack:publish-header-metas command, just as we discussed.

Merge Request : #5619

Thank you for your patience. Looking forward to the merge. 😊

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

4 participants