- Issue created by @roderik
- Status changed to Needs work
2 months ago 6:16pm 18 September 2024 - 🇳🇱Netherlands roderik Amsterdam,NL / Budapest,HU
Pushed fix for "On non-default CE displays" fixed, but it will conflict with ✨ Configurable tag name for elements built using layouts Needs review once that's merged. The rest still needs doing.
- 🇳🇱Netherlands roderik Amsterdam,NL / Budapest,HU
I've force-pushed over the first commit, which was incomplete and needed a broader solution.
This code is convoluted, arguably ugly and hard to maintain. It took me ages to structure and comment the logic in a somewhat-understandable/maintainable way and manually test/tweak the result. I still think it was necessary to spend time on it, because
- Having "Use layout builder" visible when no view mode uses it, is confusing.
- Having options invisible when in fact they do make a difference (for at least some of the view modes that use this CE display while building), is unacceptable.
I blame the convoluted-ness on the structure of configuration. (Having two separate 'layers' of displays each with their "Use Layout Builder" settings to check, combined with default/fallback displays that are used by several view modes, leads to a lot of different things to check / make clear to the user.) I see no way to make that easier and still be able to edit things when we need to.
So, it is what it is.
I'm setting this to "needs review" for a while as a sign that the MR is ready, but I'm not going to have anyone review this. It's 'only' form code, and the actual behavior is better than before.
I had counted on reusing EntityCeDisplay::getConfigDependencyEntityViewDisplays() for some of the logic I needed. But the logic turned out to not be the same, and actually this just had me doubting whether its logic is sound in the first place. So
- I added more comments to EntityCeDisplay::getConfigDependencyEntityViewDisplays() without changing the logic
- I made the method protected and removed it from the interface. This should not have any effect on classes that (theoretically) extend the interface / EntityCeDisplay. I'll publish a Change Record just for completeness. I'll comment in 🐛 Config dependencies are not properly calculated Needs work too, that my attempt at being 'smart' has failed.
This added code has no automated tests and no plans for them, "because only UI anyway". Existing tests were extended in this MR, only because I started to doubt how things worked, while coding... and the tests act as a specification of existing behavior.
I plan to merge this when I have used this myself more, in upgrading an internal project from CE 3.0.0-beta1 to this new code.
Automatically closed - issue fixed for 2 weeks with no activity.