FLUID-4500 improvements

Li, Cindy cli at ocad.ca
Wed Oct 5 17:28:24 UTC 2011


Thanks for the feedback, Antranig.

I've re-constructed the code and sent out the pull request for review: https://github.com/fluid-project/infusion/pull/184

Thanks.

Cindy

On 2011-10-05, at 4:20 AM, Antranig Basman wrote:

> Reviewing:
> 
> https://github.com/cindyli/infusion/blob/2817709833c806feede55785e88788d1da22d50c/src/webapp/components/uiOptions/js/UIEnhancer.js
> 
> The implementation has been somewhat cleaned but could be more regular, as well as having a test case.
> 
> It is probably cleaner to expand the signature of calcInitSize to (container, fontSizeMap) so that it is clear that only the main "set" driver reads and writes that.initialSize.
> 
> When I was talking about having a flag value to represent whether the container was ready, I was thinking of a separate value rather than multi-tasking initialSize - given we want initialSize to just be the return value of calcInitSize it is simpler to just revert to using the natural value of 0 for now.
> 
> Once calcInitSize has fewer side effects, it will be possible to write a test case just testing that function. White-box testing will be ok here, sending it a dummy container of the form
> [{currentStyle: {lineHeight: 10}}] etc. to simulate IE, and a genuine jQuery to simulate the other browsers.
> 
> A reasonable impl for "set" would be
> if (!that.initialSize) {
>    that.initialSize = that.calcInitSize(); // container, fontSizeMap
>    }
> if (that.initialSize) {
>    etc.
> 
> This is longer than we want but is regular enough it should be clear what to do when it comes to "hoist" impl into "ants". The best implementation would have each ant with a standardised "isContainerReady" boolean flag, together with a "measureContainer" method which returns both the ant-specific "initialSize" as well as the standardised "isContainerReady" flag. But the above will do for now.
> 
> calcInitSize loses all references to that.initialSize and has final line:
> return fluid.uiEnhancer.numerizeLineHeight(lineHeight, fontSize)
> 
> The guards can go into the impl of numerizeLineHeight - although do we really imagine there is a situation where lineHeight has a value but fontSize does not?
> 
> Similarly the "times && times > 0" branch on line 409 can be simplified - it is the responsibility of the caller to provide a numeric value here.
> 
> textSizer's "calcInitSize" should be rationalised similarly. Functions which unexpectedly modify their arguments should be discouraged.



More information about the fluid-work mailing list