Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

[11.x] allow guessing of nested component #52669

Draft
wants to merge 4 commits into
base: 11.x
Choose a base branch
from

Conversation

browner12
Copy link
Contributor

sometimes components will be part of a larger component grouping, and you may wish to group those components in a folder.

as a simple example we'll use the classic "Card" component, which may be organized as such:

  • App\View\Components\Card\Card
  • App\View\Components\Card\Header
  • App\View\Components\Card\Body

to utilize these in your Blade file you would:

<x-card.card>
    <x-card.header>Title</x-card.header>
    <x-card.body>lorem ipsum</x-card.body>
</x-card.card>

while this is fine, it doesn't read as nice as it could. with this commit, we can now omit the trailing duplicated word, as long as the class name matches the folder name.

<x-card>
    <x-card.header>Title</x-card.header>
    <x-card.body>lorem ipsum</x-card.body>
</x-card>

we do something similar with Anonymous components

If a component at the root level exists with the same name as the folder, it will take precedence.

Tried to write a test for this, but was honestly a little lost.

sometimes components will be part of a larger component grouping, and you may wish to group those components in a folder.

as a simple example we'll use the classic "Card" component, which may be organized as such:

- `App\View\Components\Card\Card`
- `App\View\Components\Card\Header`
- `App\View\Components\Card\Body`

to utilize these in your Blade file you would:

```blade
<x-card.card>
    <x-card.header>Title</x-card.header>
    <x-card.body>lorem ipsum</x-card.body>
</x-card.card>
```

while this is fine, it doesn't read as nice as it could. with this commit, we can now omit the trailing duplicated word, as long as the class name matches the folder name.

```blade
<x-card>
    <x-card.header>Title</x-card.header>
    <x-card.body>lorem ipsum</x-card.body>
</x-card>
```

we do something similar with [Anonymous components](https://laravel.com/docs/11.x/blade#anonymous-index-components)
@bert-w
Copy link
Contributor

bert-w commented Sep 7, 2024

This feels like magic; how about creating App\View\Component\Card.php so you dont have this issue?

@browner12
Copy link
Contributor Author

@bert-w the point of this is to avoid that, and be able to group you components appropriately. it's the same as what we do with anonymous components.

@bert-w
Copy link
Contributor

bert-w commented Sep 7, 2024

hm this does introduce a conflict however if you have both App\View\Component\Card.php and App\View\Component\Card\Card.php defined.

@taylorotwell taylorotwell marked this pull request as draft September 9, 2024 14:25
@taylorotwell
Copy link
Member

Think we would probably want a test here.

@browner12
Copy link
Contributor Author

@bert-w correct. I addressed that in my original comment.

someone please check my work on this test. I was not able to figure out how to get it to work with the aliases

@browner12 browner12 marked this pull request as ready for review September 9, 2024 20:40
@devajmeireles
Copy link
Contributor

@browner12, I have a sincere question! Won't we have some kind of "conflict" with this PR? I mean, your example:

<x-card>
    <x-card.header>Title</x-card.header>
    <x-card.body>lorem ipsum</x-card.body>
</x-card>

... won't it result in the component being understood as anonymous, possibly invoking card/index.blade.php if exists?

@browner12
Copy link
Contributor Author

From how I'm reading the class, the anonymous check happens after my check, and all the other class based checks.

Let me know if you're reading it differently.

However, you might be able to consider this a BC break (?) under the following scenario:

  • /App/View/Components/Card/Card.php
  • /resources/views/components/card/index.blade.php
  • /resources/views/components/card/header.blade.php
  • /resources/views/components/card/body.blade.php
  • /resources/views/components/card/card.blade.php

Prior to the change if you included <x-card></x-card>, it would anonymously render the index.blade.php view. After the change, it would use the Card.php class.

Although that seems like a very unlikely contrived example. I'll leave it to @taylorotwell to judge if that justifies pushing to 12.x

@taylorotwell
Copy link
Member

@browner12 do you think it's possible to make anonymous components consistent? Right now we require the anon component to be named index instead of the name of the folder. Could we support the folder name convention for anon components too or does that have bigger BC implications?

@taylorotwell taylorotwell marked this pull request as draft September 18, 2024 17:30
@browner12
Copy link
Contributor Author

@taylorotwell it definitely looks possible to support the folder name convention for anonymous components. I believe we would just need to adjust the guessAnonymousComponentUsingNamespaces and guessAnonymousComponentUsingPaths methods.

It seem unlikely, and very odd that someone would intentionally use both the "index" and "folder" name in their view folder for anonymous components as it doesn't make a ton of sense semantically.

  • /resources/views/components/card/index.blade.php
  • /resources/views/components/card/card.blade.php
  • /resources/views/components/card/header.blade.php
  • /resources/views/components/card/body.blade.php

However, I believe we would still be okay as long as we give the index view higher priority than the card view in the compiler. This would cause the <x-card> component to still render the index.blade.php view if it exists, and only render the card.blade.php view when the index.blade.php doesn't exist.

I will say I don't use the alias feature for 3rd party custom components, so I'm not sure if that would be affected any differently.

I would propose we add this feature in a separate PR if you'd like to go forward with it.

@browner12 browner12 marked this pull request as ready for review September 18, 2024 17:49
@taylorotwell
Copy link
Member

I think I would rather see it in this PR if that's alright - since I wouldn't merge this one without the other change because of the inconsistency.

@taylorotwell taylorotwell marked this pull request as draft September 19, 2024 11:20
@browner12
Copy link
Contributor Author

gotcha, fair. I'll work on that.

# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants