InlineEdit API change?

[Antranig Basman]

Thanks, michelled and acheetham for dealing with this issue. I noticed that part of the implementation, in 
particular InlineEdit.js lines 336-340

         // if old 'defaultViewText' option was used intead of strings version, update strings version
         if ((that.options.defaultViewText !== undefined) &&
             (that.options.strings.defaultViewText === fluid.defaults("inlineEdit").strings.defaultViewText)) {
             that.options.strings.defaultViewText = that.options.defaultViewText;
         }

could have been expressed better and left a note on
http://issues.fluidproject.org/browse/FLUID-4725

although given the comments relating to "default value merge policies" I wasn't convinced that this 
justified reopening the issue. However, these have been a supported (although poorly advertised) part of the 
framework for a long time and I expect it would be better to consider that they are indeed recommended 
rather than not.

In reading through the changes I also noticed
http://issues.fluidproject.org/browse/FLUID-4732
which is a long-standing issue that we have forgotten about/overlooked for quite a while and which we should 
probably deal with quickly, perhaps after a little channel discussion next week when more people are back.

Cheers,
Antranig

On 16/07/2012 15:44, Antranig Basman wrote:
> 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?
>>
>
>
> _______________________________________________________
> fluid-work mailing list - fluid-work at fluidproject.org
> To unsubscribe, change settings or access archives,
> see http://lists.idrc.ocad.ca/mailman/listinfo/fluid-work
>