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

avoid collision with component name property #131

Merged
merged 3 commits into from Aug 7, 2019

Conversation

@faustbrian
Copy link
Contributor

commented Aug 6, 2019

1️⃣ Is this something that is wanted/needed? Did you create a feature-request issue first?
Yes #130

2️⃣ Does it contain multiple, unrelated changes? Please separate the PRs out.
No

3️⃣ Does it include tests if possible? (Not a deal-breaker, just a nice-to-have)
Yes

4️⃣ Please include a thorough description of the feature/fix and reasons why it's useful.
At the moment the base component has a very generic property called $name which can be used to modify the component name. This could easily be overwritten and hard to figure out why a wrong component name is now used to resolve the view name.

The newly introduced getName() method allows the use of a custom name that will be used by name() and apply all the same methods as perform like diffing and imploding. This should avoid issues with components that themselves use a $name property.

5️⃣ Thanks for contributing! 🙌

@faustbrian

This comment has been minimized.

Copy link
Contributor Author

commented Aug 6, 2019

@calebporzio thinking about it now looking at the code. Shouldn't it be enough if we completely remove the name property and add some docs for how to overwrite name() to use a custom component name?

That way the default assumption of a component would be that you follow the Livewire naming and location conventions and if you don't you will overwrite the name() method.

This could in the future maybe be expanded by allowing to use a global custom name resolver.

@calebporzio

This comment has been minimized.

Copy link
Owner

commented Aug 6, 2019

@faustbrian - that makes a lot of sense.

Should we remove the $name property AND rename ->name() to ->getName? It would feel similar to a Model's ->getTable(). What do you think?

@faustbrian

This comment has been minimized.

Copy link
Contributor Author

commented Aug 7, 2019

Sounds good to me, I will make the change in an hour when I am at a computer.

@faustbrian

This comment has been minimized.

Copy link
Contributor Author

commented Aug 7, 2019

@calebporzio renamed name() to getName() and added another test to make sure default and custom names work.

@calebporzio

This comment has been minimized.

Copy link
Owner

commented Aug 7, 2019

Perfect - great PR thanks @faustbrian !

@calebporzio calebporzio merged commit a30a884 into calebporzio:master Aug 7, 2019

2 checks passed

continuous-integration/styleci/pr The analysis has passed
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details

@faustbrian faustbrian deleted the faustbrian:feat/get-name branch Aug 7, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
2 participants
You can’t perform that action at this time.