Huh? Why would the subclass call super.createElement() and assume the element will not be created?
FWIW, super.createElement() is barely called, and when it is (from all the cases I’ve found so far in the whole Basic and MDL), it’s expecting the default div element. > On Sep 26, 2017, at 8:15 PM, Alex Harui <aha...@adobe.com.INVALID> wrote: > > IIRC, UIBase has that code so if you do "new UIBase()" you will get an > element, but if subclasses call super.createElement() by habit, we won't > overwrite anybody's element. It isn't truly PAYG, it depends on how folks > feel about "requiring" that you know not to call super.createElement() on > UIBase. > > I don't have an opinion on what is "right". > > -Alex > > On 9/26/17, 8:54 AM, "Harbs" <harbs.li...@gmail.com> wrote: > >> 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.li >>>>>> nk >>>>>> e >>>>>> >>>>>> din.com%2Fpiotrzarzycki&data=02%7C01%7C%7C6716901213624a0e804708d504e2 >>>>>> 03 >>>>>> 9 >>>>>> >>>>>> f%7Cfa7b1b5a7b34438794aed2c178decee1%7C0%7C0%7C636420291136295544&sdat >>>>>> a= >>>>>> f >>>>>> Q1CjLGkCpNKxSQBmZn%2BnKZEplQpGl25XACOqq0gO2o%3D&reserved=0 >>>>>> >>>>>> >>>>>> <https://na01.safelinks.protection.outlook.com/?url=https%3A%2F%2Fpl.l >>>>>> in >>>>>> k >>>>>> >>>>>> edin.com%2Fin%2Fpiotr-zarzycki-92a53552&data=02%7C01%7C%7C671690121362 >>>>>> 4a >>>>>> 0 >>>>>> >>>>>> e804708d504e2039f%7Cfa7b1b5a7b34438794aed2c178decee1%7C0%7C0%7C6364202 >>>>>> 91 >>>>>> 1 >>>>>> >>>>>> 36295544&sdata=LzIej2n6WVnm9AG1Hi4NqIZjOQS%2Byo4w%2BPYTX0Rq8Gc%3D&rese >>>>>> rv >>>>>> e >>>>>> d=0> >>>>>> >>>>>> GitHub: >>>>>> >>>>>> https://na01.safelinks.protection.outlook.com/?url=https%3A%2F%2Fgithu >>>>>> b. >>>>>> c >>>>>> >>>>>> om%2Fpiotrzarzycki21&data=02%7C01%7C%7C6716901213624a0e804708d504e2039 >>>>>> f% >>>>>> 7 >>>>>> >>>>>> Cfa7b1b5a7b34438794aed2c178decee1%7C0%7C0%7C636420291136295544&sdata=W >>>>>> eI >>>>>> l >>>>>> LzVsJzRKniD1r9F2xb%2BwljhCLHnurBnl03kBM9E%3D&reserved=0 >>>>> >>>> >>> >> >