Hi Harbs,
I'm adding Royale dev list.
I just checked your changes with more complex app and apart of many
conflicts during merge with my custom SDK I don't see any problems. Take a
look into my comments to one of your commit. Once you do that from my sight
is green light to merge it to develop.
I'm sorry for a long delay!
Thanks, Piotr
2017-09-26 21:01 GMT+02:00 Piotr Zarzycki <piotrzarzyck...@gmail.com>:
> Harbs,
>
> Give me couple of days and I will pick up that branch and try it out. I
> will also review those changes and give the feedback.
>
> Thanks!
>
> 2017-09-26 20:50 GMT+02:00 Harbs <harbs.li...@gmail.com>:
>
>> I think I’m done. Any reason to not merge into develop?
>>
>> > 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%7C6716901213624a0e80470
>> 8d504e2
>> >> 039f%
>> >>>>>> 7
>> >>>>>> Cfa7b1b5a7b34438794aed2c178decee1%7C0%7C0%
>> >> 7C636420291136295544&sdata=WeI
>> >>>>>> l
>> >>>>>> LzVsJzRKniD1r9F2xb%2BwljhCLHnurBnl03kBM9E%3D&reserved=0
>> >>>>>
>> >>>>
>> >>>
>> >>
>> >>
>> >
>> >
>> > --
>> >
>> > Piotr Zarzycki
>> >
>> > mobile: +48 880 859 557 <+48%20880%20859%20557>
>> > skype: zarzycki10
>> >
>> > LinkedIn: http://www.linkedin.com/piotrzarzycki
>> > <https://pl.linkedin.com/in/piotr-zarzycki-92a53552>
>> >
>> > GitHub: https://github.com/piotrzarzycki21
>>
>>
>
>
> --
>
> Piotr Zarzycki
>
> mobile: +48 880 859 557 <+48%20880%20859%20557>
> skype: zarzycki10
>
> LinkedIn: http://www.linkedin.com/piotrzarzycki
> <https://pl.linkedin.com/in/piotr-zarzycki-92a53552>
>
> GitHub: https://github.com/piotrzarzycki21
>
--
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
