Manage Composer height with overridable methods

[clarkwinkelmann]

This change aims to solve issues I encountered while working on https://github.com/flagrow/mason

With the current implementation it's really hard to change the computation of the default height, as wekk as change the hard-coded minimum height of 200.

This PR separate these concerns into a separate method whose methods can be easily overridden by extensions.

I placed the class in utils, not sure if it's the best place.

I'm not sure if much performance is actually gained by using a computed property. The computedHeight method is only called inside updateHeight (my PR changes nothing to that fact), which means it's not called so often.

Removing the computed call entirely (this means ignoring the second commit of this PR), actually has the benefit of updating the height when the window is resized as well. This prevents the composer from going out of the screen if the browser height is reduced.

[tobyzerner]

Awesome work!

I'm not so sure about extracting this logic into a util class, as there is no use-case for reusability, the purpose is really quite specific to the Composer component – and especially since the logic depends on the Composer's position property, as well as the Composer component's DOM element itself.

But I would definitely support having these refactored methods be moved inside of the Composer component 👍

[clarkwinkelmann]

Should I move the class into Composer.Height, in the same fashion as there is a Composer.PositionEnum ?

Or should I just implement all these methods as members of Composer ? There's already a lot of stuff in there, keeping it as a class might help readability?

[tobyzerner]

I personally think directly as members of Composer. I know it's a large class, I just think extracting this stuff feels a bit forced/unnatural, you know what I mean? I feel like the height logic is as integral part of the component. There might be other stuff that's a better fit for extraction to make the class more readable.

But maybe we should get a second opinion from @franzliedke :D

[clarkwinkelmann]

In PHP I'd suggest an interface, but... meh :D

[clarkwinkelmann]

I moved all my changes back to the Composer class.

@tobscure what do you think about dropping the computed property ? As I said above, the method is already nearly only called when the height or position changed (given it's only called inside updateHeight, and not in view)

Also this event currently does absolutely nothing, given the window resize does not trigger a new computed value: https://github.com/flarum/core/blob/40ebc13292b64271418aa3d5683d0927e6db778e/js/forum/src/components/Composer.js#L125

[franzliedke]

Good job! 👏

[tobyzerner]

what do you think about dropping the computed property ? As I said above, the method is already nearly only called when the height or position changed (given it's only called inside updateHeight, and not in view)

Good call, the computed property is probably a case of premature optimization. Let's drop it.

Also this event currently does absolutely nothing, given the window resize does not trigger a new computed value

Without the computed property, we should get a different value for maximumHeight on window resize. Also the $flexible height in updateHeight could be different with different CSS breakpoints for the window width.

[tobyzerner]

@clarkwinkelmann status on dropping the computed property?

[clarkwinkelmann]

@tobscure oops, missed the fact you approved removing it. I'll make the changes soon

[clarkwinkelmann]

I made the final fixes, and also merged the changes to the Composer from master.

The method to get the height to apply is still called computedHeight, even if it doesn't use the computed() helper anymore. I couldn't find a better name. It's still the height computed on several factors so it still makes sense. It's also less rewriting because any call to the method can be left as-it.

PS: as per the contribution guide I didn't commit any change to the dist files, however the dist files from master did change when I merged, but I'll guess you'll take care of recompiling anyway.

[franzliedke]

@tobscure Please squash-merge if you're happy with this.

[tobyzerner]

Thanks for this @clarkwinkelmann, well done.

For future reference the contribution guide is out of date and we do prefer committing changes to the dist files.