search

enso-ui / ui

Laravel Enso UI
MIT License
9 stars 10 forks source link

Fix component reusability regression by Vue router #5

Closed jlsjonas closed 5 years ago

jlsjonas commented 5 years ago
works fine without key except for some rare edge-cases. Having the key set to path prevents component reusability if the path changes. v2 had this set to `JSON.stringify(this.$route.params)` which usually resulted in an empty object (thus not causing issues) p.s.: looks like enso-ui org rights/it's repos are set differently than laravel-enso's, couldn't commit to the repos or set metadata like meta & projects
aocneanu commented 5 years ago

p.s.: looks like enso-ui org rights/it's repos are set differently than laravel-enso's, couldn't commit to the repos or set metadata like meta & projects

I'll look into it

Regarding the key, it's the intended behaviour. We want the user to get the feedback (change page transitions) when the model is changed.

Not to mention that your fix is breaking basic usage, like when you're looking at 'administration/users/1/edit', and change the param from 1 to 2 the router does not know to rerender Edit.vue and you will look at the old model, unless doing a manual fetch.

jlsjonas commented 5 years ago

Not to mention that your fix is breaking basic usage

Please see https://router.vuejs.org/guide/essentials/dynamic-matching.html#reacting-to-params-changes

Components which depend on parameters should implement a beforeRouteUpdate (to, from, next) {... exactly for this purpose.

aocneanu commented 5 years ago

Exactly for this purpose, IF you want component reusability, which I already mentioned that is not the case.

jlsjonas commented 5 years ago

Why wouldn't you? It's a big performance win for little extra effort. It wasn't clear that you didn't want it.

If you still believe component reusability shouldn't be considered this can be closed and I'll patch it out, or revert it using patch-package.

Alternatively I can update it to use the old method again based on params, which would still allow reusability in custom scenarios.

aocneanu commented 5 years ago

I only said that it's the intended behaviour and not something to fix. I didn't said that it the best behaviour.

It looked strange given the flow of the app that I got used to, once I used searchable and switched between two users in the (same) edit page, not having the app's visual feedback.

@gandesc, @y0net , @ionutcatalinsandu , @vmcvlad , thoughts on this?

jlsjonas commented 5 years ago

I didn't said that it's the best behaviour.

Glad we're agreeing on that :-)

It looked strange... not having the app's visual feedback.

Note that a fade/similar should still be possible without much extra effort when implementing beforeRouteUpdate (&/or similar) correctly. which gives the visual feedback that might be missing while still gaining the benefits (I'd even say this is the preferable implementation method; haven't (had to) switch(ed) between 2 edit forms directly so I see what you mean)

My use-case/encounters already had visual feedback out of itself as it's on a datatable (archive mode button); since the tabledata is changing it's already showing a loading indicator while working; giving the visual queue.

ionutcatalinsandu commented 5 years ago

In my opinion we can all agree that the big performance win is actually minuscule :-), since we are talking about components that are not so cumbersome. On another note, https://vuejs.org/v2/api/#key states explicitly the purpose of key attribute: Properly trigger lifecycle hooks of a component and Trigger transitions, both things which we desire. I cannot comprehend the reason to implement beforeRouteUpdate and other such methods, to gain something which we already have. Should the need for performance arise, we will update our coding patterns accordingly taking into account this suggestion and maybe think of others.

jlsjonas commented 5 years ago

I suppose the new method is mainly an issue for vuetable components; the transition just feels wrong when going between 2 (url-defined) filters.

aocneanu commented 5 years ago

In the end I'm not going to merge this.

I guess you can easily add a custom router component and use that in the page routes where you intend reusing the rendered components.

For all the existing pages that we have across our projects adding this change would mean needing extra logic to control the fetched models, and probably recreating the transitions, and we def don't want that.