Inline Edit Code Review Results

[Michelle D'Souza]

Hi,

Mike and I have completed our long and detailed tour through all the changes that have been made to the Inline Edit component and demos for 1.3, while Anastasia took a look at the styling changes.  I've included our findings below. As a result of the reviews, several JIRAs can now be closed while others require some small changes or some functional review. There are also a few things that we don't need to do for 1.3 but should likely be put onto a roadmap for sometime in the future. 

These are the issues that are now closed:

http://issues.fluidproject.org/browse/FLUID-3810
http://issues.fluidproject.org/browse/FLUID-3800
http://issues.fluidproject.org/browse/FLUID-3786
http://issues.fluidproject.org/browse/FLUID-3785
http://issues.fluidproject.org/browse/FLUID-3769
http://issues.fluidproject.org/browse/FLUID-3760
http://issues.fluidproject.org/browse/FLUID-2652


These require functional review:

http://issues.fluidproject.org/browse/FLUID-3855 (Jonathan?)
http://issues.fluidproject.org/browse/FLUID-3752 (Jonathan?)
http://issues.fluidproject.org/browse/FLUID-3801 (James?)


These require fixes: 

http://issues.fluidproject.org/browse/FLUID-3821
 1)  The tests have a lot of repeated setup code. Perhaps we should create a setup function for these tests to remove the repetition. 
 2)  showDefaultViewText contains a line that does nothing and should be removed (line 199) 
 3)  setupTooltipTitle is a single line function and the implementation can be simplified. There is no need to wrap the element since the function is always passed a jquery object. Perhaps we should remove the function and set the title inline instead. (line 271)
 4) setupDisplayView is a confusing name since we also have a 'displayView' subcomponent that refers to something else. This is in the public API so we do need to be careful with what we name it. (line 337)
 5) It seems that the richTextViewAccessor should be moved into the integrations file. (line 766)
 6) renderKeyboardTooltip was renamed to be editModeInstruction but a couple references are still in the Integrations file (line 242)         


http://issues.fluidproject.org/browse/FLUID-3801
 1)  defaultViewStyle is meant to style an empty InlineEdit field but we no longer ship a style that does this. (line 835 in the javascript code)
 2)  we should rename fl-inlineEdit-underline and fl-inlineEdit-inlineBlock to communicate the state we are styling instead of the implementation details of the class


http://issues.fluidproject.org/browse/FLUID-3752
 1) The simple text demo contains initInlineEdit which is simply a wrapper for inlineSimpleEditSetup. We should just rename inlineSimpleEditSetup and get rid of initInlineEdit. 
 2) The CSS selectors being used in the demos (both simple and rich text) should be namespaced with 'demo'

 
Here are some larger issues that we should consider fixing in the future:

 1) Strings containing markup snippets are scattered throughout the code base. We have talked about changing InlineEdit to have a template which would remove all this hardcoded, difficult to maintain markup. 
         
 2) The API for InlineEdit includes 'paddings'. We should see if we can actually accomplish the same thing through CSS since it will be simpler. The problem that we will face if we do this is backwards compatibility. (line 857)

  3) The idea of creating a trigger guard seems to be generally useful functionality. We should consider pushing it into the framework. (line 713)


Please comment if you feel that any of the changes above should not be done.

Thanks,

Michelle


------------------------------------------------------
Michelle D'Souza
Software Coach, Fluid Project
Inclusive Design Research Centre