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. _______________________________________________________ fluid-work mailing list - [email protected] To unsubscribe, change settings or access archives, see http://lists.idrc.ocad.ca/mailman/listinfo/fluid-work
