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
