ecamp3 icon indicating copy to clipboard operation
ecamp3 copied to clipboard

ActivityResponsibles: activity chagned -> reload responsibles

Open pmattmann opened this issue 3 years ago • 2 comments

fixes #3025

pmattmann avatar Oct 16 '22 14:10 pmattmann

Couldn't we solve this simpler by adding :key="activity._meta.self" when calling <activity-responsibles>?

I don't think we want to use the component somewhere, where activity should really be reactive and watched for. The problem seems to come from the fact, that Vue tries to reuse the DOM and reuses the component, if no key is provided.

Interesting to check if we have similar bugs, where we rely on code inside mounted().

That might work - but this means each developer using the ActivityResponsibles-Component have to know it's inside. This does not look like good practice to me.

pmattmann avatar Oct 18 '22 18:10 pmattmann

That might work - but this means each developer using the ActivityResponsibles-Component have to know it's inside. This does not look like good practice to me.

The ActivityResponsibles-Component definitely gets more robust like this, so fine for me.

However, I'm not that worried about someone using the ActivityResponsibles component at another place, where it could work very well even without setting a key. I'm more worried about someone introducing another component that is used on the Activity header and running into the same issue.

For example, we use an ApiTextField for the location: I'm happy that this one works out of the box. However, it was never really designed and tested with the thought, that one could dynamically change fieldname or uri on an ApiTextField.

In that sense we're fixing a symptome, where the underlying issue probably is that we shouldn't reuse the components on the Activity view. We could try pack the complete Activity in a separate component and load it with a key. (Something for a separate PR though)

Alternatively, we'd need to make sure we prohibit any state changed activity in the mounted lifecycle. Also ok, but could become quite an effort.

usu avatar Oct 19 '22 05:10 usu

I tried to add my thoughts in code form 😄

Most relevant is the last commit https://github.com/ecamp/ecamp3/pull/3095/commits/9462efe504fe70ea80e74333e094b72cca7c86c0

The other one has no code change other than separating the view into 2 components.

Now, complete activity is reloaded, whenever a new scheduleEntry is displayed. However, it's not awaited, so cached data is already displayed eagerly (unless no data is the cache, of course).

@pmattmann: Can you have a look at this if this makes sense to you.

usu avatar Nov 06 '22 09:11 usu

@usu I'm happy with splitting into two components. But the data-reloading part is not working for me.

If you have a day with activities - open one activity in two browser-tabs. Both have loaded the activity-data. Add an ActivityResponsible on one tab. Switch to the other activity and back on the other tab (by the day-overview on the left side; not leaving the view).

If I try this, the new created ActivityResponsible is not displayed.

pmattmann avatar Nov 06 '22 13:11 pmattmann

Good catch, seems I didn't test this one good enough. Fixed with https://github.com/ecamp/ecamp3/pull/3095/commits/a1c8b511dd49ff57a1718967189e228b2e6887bc

Good testing appreciated. There are always some edge cases around dirty flag handling.

usu avatar Nov 06 '22 14:11 usu

Now it is working - perfekt. Thx for the support.

As I opened the PR, I can not approve it (but I would).

pmattmann avatar Nov 06 '22 16:11 pmattmann