FYI: TableRowItemRenderer in MDL has a separate positioner.

> On Sep 26, 2017, at 7:01 PM, Piotr Zarzycki <piotrzarzyck...@gmail.com> wrote:
> 
> Harbs,
> 
> Please push those changes into separate branch "feature/" no matter how non
> serious it look. I hope your changes will simplify things.
> 
> Thank you!
> 
> 
> 2017-09-26 17:54 GMT+02:00 Harbs <harbs.li...@gmail.com>:
> 
>> I’m working on refactoring this.
>> 
>> Is there a reason for the null check in UIBase.createElement()?
>> 
>> Why would createElement be called if the element is already created? None
>> of the subclasses have this null check.
>> 
>> if (element == null)
>>    element = document.createElement('div') as WrappedHTMLElement;
>> 
>> Do you think it’s safe to remove the check?
>> 
>>> On Sep 26, 2017, at 6:42 PM, Alex Harui <aha...@adobe.com.INVALID>
>> wrote:
>>> 
>>> I believe there are components where more than one HTMLElement is created
>>> but only one is (and can be) assigned as "element" but all have
>>> flexjs_wrapper assigned to the wrapping IUIBase.
>>> 
>>> If in fact no components need a separate positioner, it is fine to remove
>>> it.  But if we keep it, even as a getter returning element, we have to
>>> make sure our code that positions things uses positioner and not element
>>> in case someone does try to override positioner some day.
>>> 
>>> As Peter mentioned, the original thinking was that element would be the
>>> HTMLElement that defines the node in the DOM that dispatches interaction
>>> events, but positioner might be some parent of the element like a Div
>> used
>>> to give the element appropriate visuals, chrome, accessory widgets, etc,
>>> like NumericStepper, ComboBox, DateField, and maybe more sophisticated
>>> components like a RichTextEditor where the "element" is a Div that gets
>>> focus and holds the text lines, but is a child of a positioner Div that
>>> also contains child buttons for bold/italic/underline.  Another example
>>> may be VideoPlayback.  The element might be some sort of video widget but
>>> the positioner might be a div that also contains child buttons for
>>> stop/pause/rewind/forward.
>>> 
>>> Of course, I could be wrong...
>>> -Alex
>>> 
>>> On 9/26/17, 6:27 AM, "Peter Ent" <p...@adobe.com.INVALID> wrote:
>>> 
>>>> @Harbs: yes on get positioner returning element. This way someone could
>>>> override the getter and return something else if it suited their needs.
>>>> 
>>>> —peter
>>>> 
>>>> On 9/26/17, 9:25 AM, "Harbs" <harbs.li...@gmail.com> wrote:
>>>> 
>>>>> I looked at MDL and I don’t see any problem there.
>>>>> 
>>>>> I’m talking about simplifying things across the board. I don’t see how
>> it
>>>>> could effect anything.
>>>>> 
>>>>> @Peter I think removing positioner might not be a bad idea, but keeping
>>>>> it and using it as a pointer to element is basically just as cheap.
>>>>> 
>>>>>> On Sep 26, 2017, at 4:12 PM, Piotr Zarzycki <
>> piotrzarzyck...@gmail.com>
>>>>>> wrote:
>>>>>> 
>>>>>> Hi Harbs,
>>>>>> 
>>>>>> If you will do such changes like moving to set flexjs_wrapper in the
>>>>>> setter
>>>>>> of element - please make it on the separate branch. Let me test with
>> my
>>>>>> app
>>>>>> whether MDL will not breaking up. I hope that we could avoid this one,
>>>>>> even
>>>>>> if I think that it seems to be quite reasonable to do that.
>>>>>> 
>>>>>> Can you for example do this only for your custom component not for the
>>>>>> global IUIBase class ?
>>>>>> 
>>>>>> Let see what Peter say.
>>>>>> 
>>>>>> Thanks, Piotr
>>>>>> 
>>>>>> 
>>>>>> 2017-09-26 15:02 GMT+02:00 Harbs <harbs.li...@gmail.com>:
>>>>>> 
>>>>>>> Yishay and I were working on drag/drop today and we were modifying
>> one
>>>>>>> of
>>>>>>> the classes you wrote for generating the drag image.
>>>>>>> 
>>>>>>> The code can be simplified by using cloneNode() and stuffing the
>>>>>>> results
>>>>>>> into the element. The thing is, it does not work until you assign the
>>>>>>> flexjs_wrapper to the element. IMO, calling the element setter should
>>>>>>> do
>>>>>>> that automatically.
>>>>>>> 
>>>>>>> On a similar note, Every IUIBase object has a positioner set. I don’t
>>>>>>> know
>>>>>>> of a single class which has a different positioner than the element.
>>>>>>> It
>>>>>>> seems to me that positioner should be a getter (which normally
>> returns
>>>>>>> the
>>>>>>> element) that’s overridden for classes which need a different one.
>>>>>>> That
>>>>>>> will save memory for every IUIBase created.
>>>>>>> 
>>>>>>> Harbs
>>>>>>> 
>>>>>>>> On Sep 26, 2017, at 3:23 PM, Peter Ent <p...@adobe.com.INVALID>
>>>>>>>> wrote:
>>>>>>>> 
>>>>>>>> The setter for element is in HTMLElementWrapper, the super class for
>>>>>>>> UIBase. The setter for flexes_wrapper is in UIBase. So if the
>> element
>>>>>>>> setter were to also set the flexjs_wrapper, it would have to be an
>>>>>>>> override in UIBase to do it. At least that¹s how I understand it.
>>>>>>>> 
>>>>>>>> Could you elaborate a little more on the issue that is raising this
>>>>>>>> concern?
>>>>>>>> 
>>>>>>>> Your question made me scan through these classes. Looking at this
>>>>>>>> code
>>>>>>> now
>>>>>>>> makes me think we can do a better and more consistent job organizing
>>>>>>>> it
>>>>>>>> for Royale. After all, having code that can be quickly understood
>> and
>>>>>>>> modified is important.
>>>>>>>> 
>>>>>>>> ‹peter
>>>>>>>> 
>>>>>>>> On 9/26/17, 7:13 AM, "Harbs" <harbs.li...@gmail.com> wrote:
>>>>>>>> 
>>>>>>>>> Currently, setting the element of a IUIBase will not work correctly
>>>>>>>>> because the flexjs_wrapper is not set. This makes it error prone
>>>>>>>>> when
>>>>>>>>> there¹s a need to work with the underlying DOM elements for HTML
>>>>>>>>> output.
>>>>>>>>> 
>>>>>>>>> I cannot think of a reason why the wrapper should not be set when
>>>>>>> calling
>>>>>>>>> the element setter. Instead of setting the flexjs_wrapper in
>>>>>>>>> createElement(), it seems to me that it should be set in the
>> element
>>>>>>>>> setter in HTMLElementWrapper.
>>>>>>>>> 
>>>>>>>>> Anyone have a reason not to do this?
>>>>>>>>> 
>>>>>>>>> Harbs
>>>>>>>> 
>>>>>>> 
>>>>>>> 
>>>>>> 
>>>>>> 
>>>>>> --
>>>>>> 
>>>>>> Piotr Zarzycki
>>>>>> 
>>>>>> mobile: +48 880 859 557
>>>>>> skype: zarzycki10
>>>>>> 
>>>>>> LinkedIn:
>>>>>> https://na01.safelinks.protection.outlook.com/?url=
>> http%3A%2F%2Fwww.link
>>>>>> e
>>>>>> din.com%2Fpiotrzarzycki&data=02%7C01%7C%
>> 7C6716901213624a0e804708d504e203
>>>>>> 9
>>>>>> f%7Cfa7b1b5a7b34438794aed2c178decee1%7C0%7C0%
>> 7C636420291136295544&sdata=
>>>>>> f
>>>>>> Q1CjLGkCpNKxSQBmZn%2BnKZEplQpGl25XACOqq0gO2o%3D&reserved=0
>>>>>> 
>>>>>> <https://na01.safelinks.protection.outlook.com/?url=
>> https%3A%2F%2Fpl.lin
>>>>>> k
>>>>>> edin.com%2Fin%2Fpiotr-zarzycki-92a53552&data=02%
>> 7C01%7C%7C6716901213624a
>>>>>> 0
>>>>>> e804708d504e2039f%7Cfa7b1b5a7b34438794aed2c178de
>> cee1%7C0%7C0%7C636420291
>>>>>> 1
>>>>>> 36295544&sdata=LzIej2n6WVnm9AG1Hi4NqIZjOQS%
>> 2Byo4w%2BPYTX0Rq8Gc%3D&reserv
>>>>>> e
>>>>>> d=0>
>>>>>> 
>>>>>> GitHub:
>>>>>> https://na01.safelinks.protection.outlook.com/?url=
>> https%3A%2F%2Fgithub.
>>>>>> c
>>>>>> om%2Fpiotrzarzycki21&data=02%7C01%7C%7C6716901213624a0e804708d504e2
>> 039f%
>>>>>> 7
>>>>>> Cfa7b1b5a7b34438794aed2c178decee1%7C0%7C0%
>> 7C636420291136295544&sdata=WeI
>>>>>> l
>>>>>> LzVsJzRKniD1r9F2xb%2BwljhCLHnurBnl03kBM9E%3D&reserved=0
>>>>> 
>>>> 
>>> 
>> 
>> 
> 
> 
> -- 
> 
> Piotr Zarzycki
> 
> mobile: +48 880 859 557
> skype: zarzycki10
> 
> LinkedIn: http://www.linkedin.com/piotrzarzycki
> <https://pl.linkedin.com/in/piotr-zarzycki-92a53552>
> 
> GitHub: https://github.com/piotrzarzycki21

Reply via email to