Skip to content
This repository was archived by the owner on Sep 10, 2019. It is now read-only.

Updates to iconic directive to broadcast when svg injection is completed #444

Merged
merged 3 commits into from
Feb 13, 2015
Merged

Conversation

soumak77
Copy link
Contributor

@soumak77 soumak77 commented Feb 7, 2015

First off, I'm new to using GitHub, so forgive me if I'm not following the proper practices for submitting requests.

I recently switched a project I built using Foundation for Apps to use HTML5 mode, mainly for prettier urls. I followed the instructions laid out in the documentation. After doing so, I noticed the iconic SVGs weren't loading properly. After some investigation I found that the issue was caused by the iconic urls only containing hash fragments. As called out in the HTML5 mode documentation:

There is one exception: Links that only contain a hash fragment (e.g. <a href="#target">) will only change $location.hash() and not modify the url otherwise.

For more info see AngularJS issue #8934. I found a solution which will rewrite all urls in FuncIRI notation to an absolute url using the specified base: https://github.com/jeffbcross/angular-svg-base. The library works great for content loaded that angular is aware of, however, the svg injector library being used in the framework does not notify angular once an svg is added to the DOM. The solution to the fix is to compile the new SVG element once it's been added to the DOM using angular's $compile.

My proposal is to have the framework broadcast a message when iconic has finished updating the DOM to include the injected SVG. Luckily, the svg injection library provides a callback for such a case. This is only required for HTML5 mode, as the hash fragment urls will work properly otherwise. The proposed solution does not directly call $compile as not all apps would need angular to be aware of the injected svg element, only those which have specified the url base in the head of index.html (i.e. apps using html5 mode).

Also included in the changes is a fix to the iconic directive for which a previous change broke backwards compatibility. The previous change caused elements which already used data-src to become invalidated and replaced with 'assets/img/iconic/undefined.svg'. That change can be moved to another PR if desired.

@soumak77
Copy link
Contributor Author

soumak77 commented Feb 7, 2015

I did forget to comment on one other thing. There are changes made to the default app.js file which are to highlight the changes required if using HTML5 mode. Those changes can be commented out or reverted.

@gakimball gakimball added this to the 1.0.3 milestone Feb 11, 2015
@soumak77
Copy link
Contributor Author

I've come to realized that it may be best for the iconic directive to simply call $compile at all times after injecting the svg. The reasoning is because $compile is required for the dynSrc attribute to operate properly. Currently the dynSrc attribute is evaluated in the prelinking phase and sets the source. If the interoplated dynSrc attribute were to change, the change is not propagated to the iconic directive and the icon is not updated. In order for the icon to update after the source change, the directive needs to be recompiled using the original content prior to first compile. I'm working on a solution now that I'll share once complete.

If the above change is made and $compile is already being called in the directive, then IMO $compile should be called after injecting the svg to make the solution complete.

@soumak77
Copy link
Contributor Author

I just added the changes required to make the iconic directive truly dynamic. The directive is now recompiled when dynSrc is updated. I've also added an additional attribute, dynIcon, which provides the same functionality as data-icon, but dynamically recompiles upon changes.

@jamieshark jamieshark merged commit 5e73e6e into zurb:master Feb 13, 2015
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Development

Successfully merging this pull request may close these issues.

3 participants