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

Feat: Add basset() helper function #85

Merged

Conversation

MrMohamedAbdallah
Copy link
Contributor

Added basset helper function mentioned in #83.

For testing, the helper function name conflicts with the helper function defined in tests/Pest.php.

Return whenever the output is DISABLED or INVALID
@promatik
Copy link
Contributor

Hey @MrMohamedAbdallah, thank you for the PR 👌
I made a few changes, if you have the time, let me know if it still work for you.

@MrMohamedAbdallah
Copy link
Contributor Author

Hi @promatik , I wanted to add tests but I was blocked by the helper function in Pest with the same name.

I also wanted to add a feature to cache the result when the user run php artisan basset:cache to make it acts like @basset directive. But I'm not sure how to go in that direction.

@tabacitu
Copy link
Member

tabacitu commented Aug 22, 2023

Ouch! Those are valid concerns @MrMohamedAbdallah :

  • we should probably make php artisan basset:cache also cache any assets that are provided using the helper, not just using the Blade directive;
  • we should add a test (that does conflict with Pest);

I'll ask @promatik to take a look this week or the next. Right now I know he's working on a feature.

@promatik
Copy link
Contributor

Yes, I'll take a look at it, we'll need to rename the current pest helper 👌 there's no issue on doing that, it's no a BC.

Replaced current pest basset helper
@promatik
Copy link
Contributor

Ok! So this one is ready from my side 👌

  1. I changed the pest helper to not conflict with the main helper.
  2. Moved Make php artisan basset:cache also cache any assets that are provided using the helper #89 to another task 👌

@tabacitu
Copy link
Member

happy-paul-rudd-gif-by-saturday-night-live

This. Is. AWESOME! I'm super-excited, thanks a lot for this @MrMohamedAbdallah & @promatik . I've just pushed a change to the README file in this PR. I've documented this new basset() helper. In fact, adding this helper has made the whole package a lot easier to understand and use, because the comparison between asset() and basset() makes everything super-freakin-clear. So I've made it the default way of using Basset 🎉 So this contribution is HUGE. Thank you so much 🙏

@promatik please take a look at Pedro's mention on https://github.com/Laravel-Backpack/basset/pull/85/files#r1307011614 - other than that I have nothing to say on this, let's merge, tag and release as Basset 1.2.0 💪

Co-authored-by: Cristian Tăbăcitu <cristian.tabacitu@digitallyhappy.com>
@promatik promatik assigned tabacitu and unassigned promatik Aug 28, 2023
@promatik promatik merged commit 55d2354 into Laravel-Backpack:main Aug 28, 2023
1 check passed
@tabacitu
Copy link
Member

Xnapper-2023-08-31-09 49 38

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

Successfully merging this pull request may close these issues.

4 participants