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

Correctly call the tailwindcss preset if the auth flag is not passed #89

Conversation

marcusmyers
Copy link
Contributor

@marcusmyers marcusmyers commented Mar 5, 2020

Due to the removal of tailwindcss-auth macro the tailwindcss macro
will always run TailwindCssPreset::installAuth(). This is in large
part because how macro's work and the UiCommand::macro('tailwindcss')
getting overwritten by AuthCommand::macro('tailwindcss').

This commit assumes that just having one macro of tailwindcss is the
acceptable use moving forward. With that said, this commit check the
command option auth flag and if it's true will correctly install all the
auth presets.

This commit also removes the call to the install() method on the
TailwindCssPreset::installAuth method as this should always be called on
the macro.

image

Due to the removal of `tailwindcss-auth` macro the `tailwindcss` macro
will always run `TailwindCssPreset::installAuth()`. This is in large
part because how macro's work and the `UiCommand::macro('tailwindcss')`
getting overwritten by `AuthCommand::macro('tailwindcss')`.

This commit assumes that just having one macro of `tailwindcss` is the
acceptable use moving forward. With that said, this commit check the
command option auth flag and if it's true will correctly install all the
auth presets.

This commit also removes the call to the `install()` method on the
TailwindCssPreset::installAuth method as this should always be called on
the macro.
@mathieutu
Copy link

Hum. I'm not sure to understand all of this right now, but I would not merge that as this is not the standard behaviour of laravel/ui and the way that standard presets are implemented.

Maybe there is a bug in Laravel/ui, but this is not to this package to fix it.

Coming back with more information.

@mathieutu
Copy link

OK, I've seen where the problem comes from. Actually there are both different, coming from laravel/ui.

I've opened issues https://github.com/laravel/ui/issues/71 and https://github.com/laravel/ui/issues/72.

In any case, I would not advice to merge this PR, as it's just a patch for bugs in laravel/ui.

@michaeldyrynda
Copy link
Contributor

Should we just go back to tailwindcss and tailwindcss-auth?

This seems like a problem with macros in general, so doesn't make sense to try and fix it in laravel/ui 🤔

@marcusmyers
Copy link
Contributor Author

marcusmyers commented Mar 6, 2020

I'll creat a new pull request to add back the tailwindcss-auth and leave this here for the sake of history. Thanks for all the input @mathieutu and @michaeldyrynda!

@mathieutu
Copy link

And I'll tell you as soon as I have some good news from Laravel team.

Because the state for now is (from Taylor):

I don't think you should try to use laravel/ui for these presets anymore.

So it can be quite a change for the plans of this package, and of all laravel-frontend-presets in general. 😅

@mathieutu
Copy link

Ok, after some exchanges with Dries (where it says they are supposed to work on laravel/ui later so no change for now), I withdraw my message, and I think this PR should be accepted and merged.

And I personally think the --auth option is a better way than tailwindcss-auth, but it's really a matter of opinion.

Thanks anyway for the work on this package, and sorry for the misunderstanding.

@michaeldyrynda michaeldyrynda merged commit 4d084c3 into laravel-frontend-presets:master Mar 7, 2020
@michaeldyrynda
Copy link
Contributor

I've used this approach with the --auth CLI option 👍

@marcusmyers marcusmyers deleted the patch-ui-command-to-work-correctly-without-the-auth-option branch March 7, 2020 03:36
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