InlineEdit API change?

[Antranig Basman]

Hi Anastasia - I think the API change in itself is harmless here since 
the function has already been documented as non-API forming. However, 
I'm against the particular change suggested here since we have so far 
been at least moderately careful to avoid "that-ist leak" in the 
InlineEdit infrastructure, restricting our utility functions to only 
expecting the minimum information in their argument lists, rather than 
leaking a reference to the entire component "that" which then makes the 
expectations of the function unclear.

What I would recommend here is to take the opportunity for a bit of 
"opportunistic refactoring". The best times for this kind of thing are 
exactly times like the present - where some additional requirement 
increases costs within the component and makes its architectural 
problems more impactful. In this case, there are some comments attached 
to some strings in the options block of this form:

         // this is here for backwards API compatibility, but should be 
in the strings block
         defaultViewText: "Click here to edit",

etc.

A good move here would be to write some code to actually copy these 
options into the strings block after initialisation, and to rewrite the 
InlineEdit implementation so that the versions in the strings block are 
the ones used throughout. You can then supply the signature to 
bindHighlightHandler as (element, displayModeRenderer, styles, strings) 
which I think would be a reasonable and communicative signature. 
Although "displayModeRenderer" is I think a quite misleading name for 
this DOM element argument.

The string options accepted at top level can then be marked as properly 
deprecated and scheduled for removal in some future version of the API 
(perhaps 2.0)

Cheers,
Antranig


On 16/07/2012 22:40, Cheetham, Anastasia wrote:
>
> I'm working on implementing a change to InlineEdit requested by the designers: the ability to change the invitation text when focus lands on the field.
>
> 	http://issues.fluidproject.org/browse/FLUID-4725
>
> The implementation requires a small API change to one of the functions, fluid.inlineEdit.bindHighlightHandler(), changing the last parameter:
>
> -    fluid.inlineEdit.bindHighlightHandler = function (element, displayModeRenderer, styles) {
> +    fluid.inlineEdit.bindHighlightHandler = function (element, displayModeRenderer, that) {
>
> This function binds keyboard focus and blur event handlers to an element. It is not part of the 'public' API, and I suspect that it's not something that is being overridden by implementations: the function basically adds and removes classes, so customization would likely occur by adjusting the styles themselves or overriding the classnames.
>
> I'm wondering how the community feels about this API change. Does anyone want to point out reasons not to make the change?
>