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%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
>
>
--
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
