Angular and the GUI

[jeffreyselby]

A bug has been identified in gui.min.js.

Problem area: File gui.min.js. Refer to the below code ...

t.init = function() {
        e.debugging("buttons: Initiating", "report"),
        $(".js-button-dropdown").length && (e.debugging("buttons: Found instances", "report"),
        $(".dropdown-menu").attr("aria-hidden", "true"),
        $(".js-button-dropdown").on("click", function() {
            e.debugging("buttons: dropdown button clicked", "interaction");
            var t = $(this)
              , n = t.parent("div")
              , i = t.next(".dropdown-menu")
              , s = n.hasClass("is-open");
            o(s, n, i)
        }
        ),
        $(document).keyup(function(t) {
            27 == t.keyCode && (e.debugging("buttons: Esc button clicked", "interaction"),
            o(!0, $(".btn-dropdown"), $(".dropdown-menu")))
        }
        ))
    }

Description: Dropdown buttons do not work if the Dropdown Button is added to the DOM after the GUI.buttons.init() function is executed in gui.min.js.

Explanation of Bug: In the gui.min.js file there is a bug with registering the "click" event to a Dropdown button. GUI Version 2 is using the following syntax ...

syntax $(selector).on(event, function)

That is ...

 $(".js-button-dropdown").on("click", function() { ... });

The problem with this syntax is that the "click" event is only bound for those elements already part of the DOM. The event is NOT triggered if the Dropdown Button is added to the DOM at a later point. Instead the following syntax should be applied.

$(selector).on(event,childSelector,data,function,map) where selector would refer to "document" i.e. a parent element.

ATTENTION:

1. The gui.min.js file should be checked for other occurrences where events will not be bound to FUTURE elements. The syntax $(document).on(event,childSelector,data,function,map) ensures that FUTURE elements will be bound to the event
2. Check the line $(".js-button-dropdown").length. This line may also prevent FUTURE Dropdown buttons being bound to the "click" event.

[dominikwilkowski]

Hi @jeffreyselby,

Thanks for the report. Binding to the document is a bad practise as it will bubble up events in your javascript even though you might not even have the element in the DOM. The JS in the GUI is designed to be as lean as possible so when you add elements dynamically to the DOM you have to run the init method of that module. That's why they all come with a public API and init() methods.

It's all about performance for us as we don't know where things are being used. This is the most modular way so the GUI can be used in all kinds of scenarios.

[dominikwilkowski]

FYI you can see the unminified JS here: https://github.com/WestpacCXTeam/GUI-source/blob/master/buttons/1.0.0/js/buttons.js

[jharris-westpac]

Hi,

The issue here is a much wider one than what has been immediately mentioned. Much of the deployments within the Westpac group are based on single page app frameworks such as AngularJS. The issue we have with GUI 2 is that it was not built around a SPA framework and heavily based on jQuery and the document onload / ready state.

The work around we have had for this in the past for GUI 1 projects is to use a 3rd party library in our frameworks to handle all of the JavaScript components. For angular and the 1 click platform / eforms projects has been UI bootstrap. The UI bootstrap library has been key to us being able to implement the GEL libraries dynamic components which are essentially bootstrap components.

The Westpac eForms team (T Wong and myself) had asked for you to maintain the HTML contract when developing any new components, however it seems this has not been upheld and much of the GEL 2 is brand new markup and no longer based on Bootstrap 3. This means we can no longer utilize the UI Bootstrap library and will have to re-implement every GEL 2 component which relies on JavaScript. The solution is that we have to port the jQuery like code in GEL 2 into the Angular framework as a directive. Looking at the source code for UI Bootstrap this is around 4,000 lines.

Because this is a significant undertaking we would need to organize a meeting between the teams on a strategy of how to go forward with GEL 2 in eForms / Web presentation kit as its new name.

Cheers.

[dominikwilkowski]

Hey James,

Great point. The GUI was build right from the start to have accessibility built into it and therefor could not be bootstrap anymore. There are lots of other reasons but that would be the main one for changing to semantic HTML.

We also have to think about all the other projects using the GUI that do not use an SPA architecture or even angular when they do so the course we decided on was to create as much in CSS as we can and only use JS to change classes or slight positioning where it's needed. You'll find that we only have 6 modules currently that use JS at all out of the 46 in total. All HTML, CSS and JS has been rebuilt and is not comparable to bootstrap anymore even though we tried to keep HTML the same and keep as many class names as well.

We also made a conscious decision to NOT bind to document.load but instead run code modular and at any time you decide to. I am not sure what you mean when you say:

The issue we have with GUI 2 is that it was not built around a SPA framework and heavily based on jQuery and the document onload / ready state.

The GUI is working with jQuery and we already have set our roadmap to convert to vanillaJS either ES6 + babel or ES5. It's more a time issue :)

As I see it working now with angular is to "just" run the init() method every time you add a component to the DOM that has js attached. It is built to be lean and quick and is currently the best way for us to manage a wide variety of flavours in front end js frameworks so everyone can use it without being forced to use a particular framework. It's basically a No-Assumptions, Just-Run sorta snippet. (jQuery being an assumption we had to make for now)

In terms of rewriting the JS, sure you can do that. the 6 modules don't have more than a couple lines of code in them but you have to maintain the accessibility functionality that was never present in bootstrap. Let's have a meeting and work through it. :+1: If there is a bug or something we can do we are very happy to create a new version and fix it. We also accept pull requests if you have something already in mind.

Thanks again James, let's keep this discourse going.

[jharris-westpac]

Hi Dominik, Great points! Thanks for the discussion and it makes this issue much clearer. You are absolutely right in that the crux of the issue is the init method, but also with Angular there is more of an 'angular' way of writing dynamic components. To re-iterate your point, those 6 modules will likely be simply swapping out classes, this is where AngularJS (And probably other SPA frameworks) are great. Angular has its own directives / handlers to deal with this in a very clean way as to not tightly couple it with the HTML. Angular also has very nice methods of adding / removing HTML. By doing it the angular way we also avoid the issue of re-binding events and memory leaks associated with that and garbage collection since angular deals with that internally.

So it looks like then, we would need to move across and angularize any module which has an init method, and perhaps have a look at any module which has a JS dependency. Off the top of my head the ones with potentially high complexity would be, tabs / accordion, modals, carousels?, date picker?, pagination, popovers and typeaheads.

Would be great to have some time to go through together and see how they have been implemented and the complexity to bring them across into Angular directives.

Thanks!

[dominikwilkowski]

Absolutely. This is going a bit off-topic as this issue is about running the events after a module has been added to the DOM dynamically.

Let's have a meeting with Andrew Dowd and yourself while Tony is away and knock this out. We have a couple ideas how we can make it easier for you.

The modules that are using JS currently are:

• alerts
• buttons
• modals
• popovers
• tabcordions
• tooltips

[jeffreyselby]

Hi Dominik,

As mentioned by James there are problems when AngularJS projects implement GUI version 2 in its current form. The project I am involved in has attempted to implement GUI Version 2. Given the fact GUI Version 2 has compatibility problems with Angular JS and Bootstrap.UI (e.g. the "Collapse" component does not function with the GUI), we will in all likelihood revert to GUI version 1.1.2. I am aware that James' area has refrained from implementing GUI Version 2 and I wish I had done likewise to avoid a wasted week.

Your suggestion of calling functions such GUI.buttons.init() to bind events on dynamically created elements is not the Angular JS way. Changes in the model should drive the view and additional method calls should not be necessary. Futher, it would not be appropriate to repeatedly call GUI.buttons.init() every time a new element (e.g. Dropdown Button) is added to the DOM. The method call affects both newly added elements AND elements which already existed before the method call. With the current implementation, if GUI.buttons.init() is called multiple times the "click" event listener toggles between on and off for those elements part of the DOM . I have formed the view that the GUI Version 2 was developed with the assumption that the DOM will be (largely) static.

It is unfortunate that there is no indication on the GUI web site that Version 2 is not suitable for Angular JS websites. Had I been aware of this earlier, I wouldn't have attempted to utilise the latest GUI. I would strongly suggest placing a notification on the web site warning that AngularJS projects should refrain from moving to Version 2.

BTW, are there LESS files available for Version 1.1.2. We need to modify the colors for its implementation?

Regards,

Jeff

[dominikwilkowski]

Hey Jeffrey,

We feel your pain. We totally get that the GUI is not angular and that it is not setup in the angular way. We cannot do that as we have a lot of other projects using the GUI that don't use angular.

But you have a valid point and it's very important to address this.

We have a couple ideas as to how we may help you setup with the design the GUI provides. We're also very open to suggestions if you'd like to contribute. Even this discussion is healthy for a product to get better.

For now we think the best course is to create something very much like the BootstrapUI package you were using and have the community maintain it. I've setup a public repo over here. This is where we'll maintain an angular port and we are in the very early stages(for now it's empty). Please have a look and let us know what you think.

Hopefully this model will be flexible enough to maintain some other frameworks in the future like react and durandal as some of the other tech teams are using those instead of angular.

I am happy to talk through other options you guys come up with though.

[BastienZag]

Hey hey,

You can still use most of the GUI with Angular. I understand that you cannot add the .js file for these 6 components because off the event handling. You will have to change them to angular directives.

But you can still reuse the .less or .css, grunt tasks, ...

The best approach will be to create directives for: alerts buttons modals popovers tabcordions tooltips

But it's going to be hard to maintain these components in more than vanilla js.

The new repo is a cool idea and the way to go.

[jharris-westpac]

Awesome work guys! Just to let everyone know, we have a meeting next Tuesday to have a look at this in detail (CX & UI). The good news is it seems everyone is clear about the issues and we have a way forward. Look forward to implementing UI-GUI ;)

[dominikwilkowski]

hey @everyone we just pushed a little test to https://github.com/WestpacCXTeam/GUI-angular

(we had to recreate the repo because of a git issue)

[dominikwilkowski]

Did you had a look @jharris-westpac and @jeffreyselby ?

[jharris-westpac]

Hi @dominikwilkowski

I had a quick look with my colleague whom im trying to get looking into this port. I have some issues with the dev branch on GUI-Angular in that he has copied the GUI 2 code over directly and placed it into a link function in the directive. Sure it works, but has 2 big issues.

1. Tight coupling with the jquery to the HTML which is what AngularJS was created for to avoid this. For instance hes made many references to elements using a jQuery selector based on class and hierarchy. This should be changed to the angular way which is define attributes in the HTML and use things like ng-class.
2. Event handling bindings. The event handlers for things like click on keypress have been copied over, however they pose a risk of a memory leak and double binding events if a user was to flick between pages or have this directive placed within an ng-repeat which means the code gets run over and over again, not just once. that means that the click event gets bound potentially hundreds of times and never removed. The closures created are not cleaned up and toggle functions will behave erratically.

This was just a quick analysis on the popover element which was there. I feel these issues could become much more complex when we deal with things like modals.

I have been tempted to just let it go and ignore the above issues as this method would be the most simple way of getting GUI 2 'working' with angular. But has issues with it. I would feel better inside if we could port it property and maintain the angular way of doing things however :)

[BastienZag]

Hi @jharris-westpac

These element does not use any jQuery, attach events on Angular object only.

And use the on destroy angular event if needed.

In the alert directive you can see the use of ngClass.

But on some other directive we use Angular to get DOM element (not jQuery). Because we sometime want to use tranclusion or because we don't want to change the structure of the DOM between the GUI and the angular gui.

Please check the Angular bootstrap ui: https://angular-ui.github.io/bootstrap/ https://github.com/angular-ui/bootstrap/blob/master/src/dropdown/dropdown.js

This is the most popular framework of directive. And they are attaching event as we do (but only on angular object).

[dominikwilkowski]

Hey guys,

I agree with @jharris-westpac with that there shouldn't be any jquery or events bubbling up. That's really pretty straight forward.

Do you feel comfortable to do the porting work inside this repository and collaborate with whom ever wants to help you or would you want to develop something behind closed doors? I am trying to find a good way for other projects to use what you guys have done here.

What do you think?

[jharris-westpac]

@BastienZag my bad! haha ok will look at the other components :)
Ideally if @dominikwilkowski has infinite time ;) I would love it hint if he could try to convert all jquery css manipulation into class switching. This would be animations, height changes etc and use CSS animations / keyframes to control them? Is this possible with infinite time @dominikwilkowski ? :) Would just make our porting work a lot easier.

[dominikwilkowski]

oh wow I love that idea,

But have a look at popovers, they inject CSS to reposition the popover in case it is too close to the edge of the browser. That won't be possible in CSS alone (keeping in mind we need IE8 support) :)

Happy to hear suggestions though. We are only talking about 6 components anyway.

[jharris-westpac]

Cool, yes thats right there is definitely a need for jQuery or raw JS to check the viewport and perhaps add a class align-left, align-top, align-right, align-bottom depending on whats the screen position? Sometimes there is no workaround like popovers I guess so we will just port all the necessary code and thats fine. But where we can classes would be nice :)

[dominikwilkowski]

100% james!

That's what we are doing now and I will refactor the js code the next GUI support day (I think next Tuesday) so please keep the preferences coming.

So far I will remove keybindings from the document and move things into a render method that won't bubble up events and leak memory when rendered multiple times.

@BastienZag think we need a destroy method as well. In the spirit of keeping things lean I may have to think outside the box and give destroys more attention in the docs.

Anything else you guys want?

[dominikwilkowski]

hey guys,

I refactored all JS now to accomplish the following:

• [x] no events attached to document or body
• [x] make sure elements already in the DOM don't leak when running init() multiple times
• [x] created a render() method you can run after you inject an element.

This caused HTML changes to three modules. (All that had to change was an added js-class.)

These changes will be on display for two weeks now before we roll it out in the next milestone. Have a look at the dev branch and let me know if you can see anything that might trouble you.

[dominikwilkowski]

Hey @jharris-westpac @jeffreyselby Have you had a look at the new refactored js?

Any comments?

[dominikwilkowski]

last call? anyone?

[jharris-westpac]

Hey @dominikwilkowski it's looking better. One more suggestion, is it possible to make all animations as CSS? As to remove all jquery .animate() calls? Also any dimension adjustments, could also be done by class switching? I think I saw somewhere it was setting an explicit height. And finally is it possible to remove any .parent().parent() calls I thiink I saw one of them in there. They scare me as its tight coupling to HTML structure. Perhaps could be changed to a class selector?

Thanks mate! Also I built the new Uber Radio (justins design) and its getting its first use, looks great.

[dominikwilkowski]

Hey @jharris-westpac

Yeah agree about the parent selectors. It would just mean we have to change HTML again ... I was trying to avoid that.

Animate calls have to stay as well as the positioning as we have to calculate the edge of the browser and repositioning the window. It's very small though. Every other module in the GUI uses CSS as a first priority.

[dominikwilkowski]

@jharris-westpac I just removed all parent() from the js code. Good call mate. Changed some HTML but better now than later ;)

via a47d63167fcd53c5bf54c9acea19c1e85b6296fb

[oksushi]

:+1:

[elisechant]

would something like $(document).on('click', '.js-button-dropdown', function()... instead help?