search

chris-ware / nova-breadcrumbs

Breadcrumbs for Laravel Nova
152 stars 44 forks source link

fix duplicate queries #34

Closed gaabora closed 5 years ago

gaabora commented 5 years ago

related to #30 (actualy there still duplicate queries, but one less)

chris-ware commented 5 years ago

Firstly, this doesn't fix anything, it breaks things. The mixin is only used on Attach, Update and UpdateAttach views. I know this hasn't been tested as the compiled JS isn't with this.

Secondly, you're looking in the wrong place. The duplicate issue is with the custom views needing to use the actual views as a mixin, which means that Vue lifecycle events run on both the custom and original view.

I'll leave this pull request open if you'd like to try and help, or you can close it if you want.

gaabora commented 5 years ago

Firstly, this doesn't fix anything, it breaks things.

no it does. If you look at your CustomUpdate.vue CustomUpdateAttached.vue CustomAttach.vue you will see that all 3 of them already have a https://github.com/chris-ware/nova-breadcrumbs/blob/63e7117171b946f588bf328d25ed41f292f83b5b/resources/js/views/CustomAttach.vue#L31

so making the same function call also in the mixin causes a unnecessery json request. And also it was tested, I did not want to create a local repo for fixing one evident bug so i edited and recompile it in a vendor folder and then edit the code in browser.

Secondly

You may be surprised of this actialy 2 different duplicated queries (4 queries instead of 2 in total) and one of them fixed with this PR

gaabora commented 5 years ago

Now all duplicate queries eliminated. (except I did not test CustomAttach.vue and CustomUpdateAttached.vue)

chris-ware commented 5 years ago

I will test this, but this all looks incredibly promising. It's a bit outside my understanding of Vue, but I get the idea of what it is trying to do, I think. In any case, thank you for your input, it's appreciated. If it all works, I'll try and push this as a new versions as soon as I can.

lvdhoorn commented 5 years ago

How was the test? If all was good can you merge it? We would love to continue using this package.

chris-ware commented 5 years ago

I've finally had chance to test this. All seems to work except for the UpdateAttached view. It seems to throw a 404 with data not being defined. I'm going to try and see I can figure it out as to what is wrong.

gaabora commented 5 years ago

Btw, @chris-ware, how did you test Attach and UpdateAttached? I kinda have not use this types of views yet (and have no idea where to navigate to find it)

chris-ware commented 5 years ago

They are BelongsTo fields in Nova. I just tested the relationships.