Hi Antranig, Could you please take a quick look to see what's up with this. Sorry that I don't have any proper unit tests yet. I've been quite distracted and haven't had a chance to come back to this in a while.
The branch: https://github.com/jobara/infusion/tree/FLUID-4209 So the issue is that for the ToC if there is a level that gets skipped, instead of being blank it is filled in by the parent content. so you would get something like Humans - Humans - - Catts Instead of Humans -- Catts You can run the demo to see this in action. https://github.com/jobara/infusion/blob/FLUID-4209/src/webapp/standalone-demos/table-of-contents/html/TableOfContents.html If you also have time to do another general review that would be great too. Thanks Justin On 2011-05-26, at 3:11 PM, Justin Obara wrote: > Thanks for the in depth code review. I'll reply to both e-mails here. Sorry > about the e-mail mix up, by the way. I think that's a good reason why I > shouldn't be sending out e-mails late at night :) Another reason for not > sending e-mails late at night is that I forgot to cc the list, which I'm > doing this time. > > Please see my replies inline below. > > Also, I'm still having the problem with the skipped level being filled by the > parent heading. See the bottom of the e-mail for an example. > > I've pushed up lots of refactoring to my github branch, but didn't get to the > unit tests yet. :( this is very sad. I'll try to get to them asap. Probably > should have started with them. > > https://github.com/jobara/infusion/tree/FLUID-4209 > > Thanks > Justin > > > On 2011-05-26, at 1:44 AM, Antranig Basman wrote: > >> Thanks for sending along your TOCCK for review, here are my comments: >> >> Line 43: This should be a call to fluid.transform rather than fluid.each. > > I should have spotted that one. This seems so obvious now that you've > mentioned it. I suppose my reasoning would have been along the lines of the > fact that it was also performing DOM manipulation to insert the anchor. > >> >> Line 44: Factor this out as a "headingTextToAnchor" function. Should >> probably distinguish throughout that "anchors" is an array of anchor URLs >> rather than just the names of them. Perhaps better to operate on a list of >> {url: url, id: id} rather than just a plain list of strings even though it >> seems a bit petty to do this just for the sake of a present or missing "#" >> :P This at least would enable you get the "insertAnchor" operation out of >> the loop which at present is an undesirable side-effect. >> >> The component entry "tocAnchors" doesn't seem to serve a clear function to >> component users as it stands... at the least, it might be better for it to >> take a form that might help users to find these controls in the DOM. By the >> sheerest accident, the selector which they would use is textually identical >> to the anchor URL, but it would be sleazy to exploit this explicitly :) > > I've removed the potential sleaze by using the object structure you > recommended above. I've also pulled out the code from within the loop into a > function called "headingTextToAnchor" and move the sanitization code into an > invoker. Also renamed tocAnchors to anchorInfo. > >> >> Line 60, etc. - there's no real benefit using the long form >> "{fluid.tableOfContents}" here since you are SURE that the short form also >> matches and there is nothing intervening. > > I think I understand what you mean here.. basically I just converted anything > that was {fluid.tableOfContents} to {tableOfContents}. I'm not sure i fully > understand when you can and cannot use the short vs long form. > >> >> Line 91: It would be better if this could be expressed as a "pure function", >> that is, one that doesn't depend on the DOM existing, since it is so close >> to this condition already. If the user has "prepared" the headings argument >> by ensuring it is just a list of strings, this function could be more >> reusable (as well as on the server). >> >> Further: >> >> Line 91: Any particular reason for this function to consume its argument, >> "headings"? I think it is probably positively immoral and might cause >> trouble by consuming an upstream value in the caller. This did happen once >> before :) Then you can just save on lines 131-132. Functions which destroy >> their arguments are "unexpected" and best not to provide for. See previous >> comment on reforming "headings" in any case. Now I read this function more >> closely I realise the previous comment needs some refinement. It may seem >> heretical, but this might be a function which is more clearly expressed >> non-recursively :) I recommend splitting into two functions as a >> non-recursive 2-step process: >> >> step 1) transform DOM headings + anchors into a flat structure [{text: text, >> url: url, level: level}] > > This is funny, I actually had a function that produced a structure that > looked identical to this. However I just merged it into the current function > to save looping through the data. I'll just resurrect the code, but make some > adjustments to it. > > https://github.com/jobara/infusion/commit/74b9097aa659a9f128eb1a0fb9a5a80350ba86d6#L0L117 > >> step 2) hierarchicalise this structure as a pure "model transformation", >> scanning through it looking for upticks and downticks in the value of "level" >> >> What happens in the case that the sequence in the document is something >> perverse like >> <h3> >> <h4> >> <h1> >> ? > > This case actually worked in the current state. This was handled by the while > loop and recursion. So the while loop would handle all headings at a give > level, with and recursion to handle lower levels. I tried, but couldn't > conceptualize how to do this without recursion. I've settled on basically > keeping the same function but moving it down a level so that it is an > internal function called within the main toModel method. This way I can > provide a buffer in between to preserve the incoming array. I did also make > some other refinements here and there. > >> >> The advantage of the refactoring I describe is that lots of test cases can >> be issued against the "step 2" function in isolation. > > It should need a DOM anymore and should be usable to run tests against. > >> >> Line 95: Slight inconsistency of style here - on this line you check >0 >> whereas on 106 you simply use falsity for what is really the case - you >> should centralise on one form (I would prefer the former) > > I switched to checking if it was > 0. > >> Line 121: any particular reason to avoid returning []? I think it is a >> reasonable encoding of a condition of "having no headings". > > I've gotten rid of that bit. > >> >> Line 124: Probably better expressed as "micro-component" with the literal >> array in defaults. You never know when someone might want to customise it >> (even though it is just 2 lines, they would probably be more comfortable >> just seeing data) > > Converted this to a component. > >> >> Line 158: I can only heartily regret this necessity as a result of framework >> deficiencies :P It is a really excellent effort at making the best of a bad >> job... line 195 is just awful though, I recommended to JURA that he not >> tried to manipulate unexpanded trees but I guess there is just not very much >> to be done. It will just have to stand as an incentive to implement ANTIGENS >> as fast as possible... >> >> Line 207: The constant 6 should not be coded here but derived from the same >> configuration we put into the headingLevel component. > > So I thought about this a bit, and I don't think it quite fits. At least not > the way I've structured the headingLevel component (which i now call > headingCalculator). All it's concerned with is telling what level a given > heading has. Also if someone were to swap in a different component here, > there could be a totally different means of calculating the level that may > not show up like an array in the configuration. Actually I might have to > write another function if we want to support HTML5 headings. Which are based > on nesting level within sections on the page, rather that specifically by > heading tag. > > It tried to think of a way to calculate this, but could really think of > anything really desirable. Please let me know if you have more suggestions > about this. For the timebeing. I've set a "maxLevel" option in the levels > componet's defaults. Figure this is at least a bit better. > >> >> Line 225: Presumably the vestigial "events" can go > > oops yes.. removed that one. >> >> The component is hugely improved over its previous implementation and should >> be much less work to convert over to its final form once the framework is >> improved. >> >> Cheers, >> Boz >> >> >> On 25/05/2011 20:24, Justin Obara wrote: >>> Hi Antranig, >>> >>> Hope you had a good day JA-SIGing. >>> >>> So I'm wondering if you will have some time to take a quick look at the >>> work I've done so far for the tableOfContents component. >>> >>> I've tried to rewrite it in the new IoC and protocomponent style, and >>> fixing up markup and etc. along the way. Please feel free to let me know of >>> any irrational, insane, and/or immoral aspects of it. >>> >>> code: >>> https://github.com/jobara/infusion/blob/FLUID-4209/src/webapp/components/tableOfContents/js/TableOfContents.js >>> >>> template: >>> https://github.com/jobara/infusion/blob/FLUID-4209/src/webapp/components/tableOfContents/html/TableOfContents.html >>> >>> A demo you can try. >>> https://github.com/jobara/infusion/blob/FLUID-4209/src/webapp/standalone-demos/table-of-contents/html/TableOfContents.html >>> >>> I have yet to fix up and expand the unit tests, but this will be done >>> before I make an official pull request. The reason for this, which >>> admittedly may not be a good one, is that from the start I had been >>> experimenting and had not really settled on anything. I've actually gone >>> back and rewritten the model builder, the prototree, and even the template >>> several times. I suppose better unit tests may have helped some of these >>> rewrites. >>> >>> Anyways, the major issue that I'm having now, is that when I have a case >>> where the headings are not in consecutive order I get one repeated to fill >>> in the gap. You can see this if you run the demo. >>> >>> example from demo: >>> >>> - Mammals >>> - Humans >>> - Humans >>> - CATTS >>> >>> should have been: >>> >>> - Mammals >>> - Humans >>> -CATTS >>> >>> I think that both the model and the prototree are correct. Although I could >>> have missed something. However, I'm a bit stumped as to why the one Heading >>> gets repeated to fill in the missing level. >>> >>> I'm hoping you can point me in the right direction to solve this issue, as >>> well as providing some guidance on the architecture of the component, or >>> more correctly collection of components, as a whole. >>> >>> Thanks >>> Justin >> >
_______________________________________________________ fluid-work mailing list - [email protected] To unsubscribe, change settings or access archives, see http://fluidproject.org/mailman/listinfo/fluid-work
