search

NathanWalker / nativescript-ngx-fonticon

Use custom font icon collections seamlessly with NativeScript + Angular.
MIT License
76 stars 39 forks source link

Update code for ns6 and update readme to correspond to new api #61

Closed jalbatross closed 4 years ago

jalbatross commented 4 years ago

Fixes https://github.com/NathanWalker/nativescript-ngx-fonticon/issues/60

We could also make it so that there are two separate ways of instantiating config key, value pairs to support the legacy flow (pre NS6). However, I think that this implementation will work for pre NS6 as well.

akempes commented 4 years ago

I would love to see this pull request to be merged as we are stuck with the same problem in NS6.

NathanWalker commented 4 years ago

@hypery2k these changes seem to not work in NS6 projects - I may need to revert all this and republish previous setup.

jalbatross commented 4 years ago

@NathanWalker

I just tested this on a clean NS6 Angular app and it seems to be working: https://github.com/jalbatross/working-ngx-fonticon

NathanWalker commented 4 years ago

@jalbatross thanks - will check with fresh project - had troubles integrating this into large existing project - the require would not work. Also curious:

let that = this;
    return new Promise(function(resolve, reject) {
      try {
        var cssData = that.config[configKey];
        that.mapCss(cssData);

any reason not to just use fat arrow there vs. that/this?

jalbatross commented 4 years ago

@NathanWalker No reason at all, fat arrow is definitely cleaner.

NathanWalker commented 4 years ago

@jalbatross I published 5.0.2 that cleaned that up to use fat arrow but also compiles the lib with Angular 8 which cleans some compilation issues up. However I ran into problems with the require usage which was added and found under some project conditions this applies: https://github.com/NathanWalker/nativescript-ngx-fonticon/issues/62#issuecomment-548233352 We may want to add note to README about this.

jalbatross commented 4 years ago

@NathanWalker

Agreed, made pr #63 to address it