aurelia / ux

A user experience framework with higher-level capabilities, designed to bring simplicity and elegance to building cross-device, rich experiences.
MIT License
368 stars 55 forks source link

feat(icon): icon map registration #235

Closed ben-girardet closed 4 years ago

ben-girardet commented 4 years ago

@EisenbergEffect @bigopon

As stated in https://github.com/aurelia/ux/pull/232#issuecomment-581082139 here is a proposal for a registration mechanism for ux-icon

Now the ux-icon contains no icons to start with but a UxIconMap static class that can handle icons registration.

Icons can be registered in the plugin configure call:

// main.ts
const icons = [
  {
    name: 'add',
    material: '...'
  },
  {
    name: 'remove',
    material: '...'
  },
];
.plugin(PLATFORM.moduleName('@aurelia-ux/icons'), icons)

or it can be registered later on:

// app.ts
import { UxIconMap } from '@aurelia-ux/icons';

UxIconMap.registerIcons(icons);

I think it could be helpful to provide a few icon sets based on the current list. I've created a sets folder with two files: minimal.ts and full.ts.

Currently I've tested to import them in an application but I need to call them like:

import { set as icons } from '@aurelia-ux/icons/dist/native-modules/sets/minimal';

I don't know how to build the icon sets so that we could import them like:

import { set as icons } from '@aurelia-ux/icons/sets/minimal';
EisenbergEffect commented 4 years ago

@bigopon may have some thoughts on improving the import situation.

In terms of the design, if possible, I'd prefer the icon map to be non-static. We can have a singleton instance injected into the icon element and we can create a configure method for the plugin that receives the icons to register with the map instance at startup. Make sense?

ben-girardet commented 4 years ago

Sure we can make it singleton. Happy to go this way. I am curious to understand the benefit of avoiding static classes. I guess that the purpose is to take advantage of the IoC?

If we make it singleton, what would be the way to register new icon sets outside of the plugin configuration ? Something like:

import { Container } from ‘aurelia-framework’;
import { UxIconMap } from ‘@aurelia-ux/icons’;

Container.instance.get(UxIconMap).registerIcons(...);
EisenbergEffect commented 4 years ago

In general, we prefer to avoid static classes due to how they affect testing or potentially multiple aurelia app instances on the same page. So, we like everything to go through the IoC container so that it can be scoped to the Aurelia instance (if possible). Outside of plugin configuration, any class could inject the registry and register icons. Any any other plugin could do that as well.

@inject(UxIconMap)
export class MyClass {
  constructor(iconMap) { ... }
} 
ben-girardet commented 4 years ago

Makes total sense, thanks for clarifying the purpose.

ben-girardet commented 4 years ago

@EisenbergEffect : https://github.com/aurelia/ux/pull/235/commits/9a6dedd40227f3ebeb3644fbeac5e9e3765a1606

ben-girardet commented 4 years ago

@EisenbergEffect fixed the Container issue.

@bigopon regarding the import question above, I'm trying another solution: using .json files for the icons maps and placing them in the main package directory. This way they will end up at the root of the package and can be imported as

import fullSet from `@aurelia-icons/sets/full.json`

What do you think ?

ben-girardet commented 4 years ago

Wanted to try to use the registration mechanism to bridge with another icon library. With FontAwesome it could look like this:

  1. Register the component without icons
// main.ts
// ...
  .plugin(PLATFORM.moduleName('@aurelia-ux/icons'))
// ...
  1. Import the FA JS icons and parse them
// app.ts

import { UxIconMap } from '@aurelia-ux/icons';
import { inject } from 'aurelia-framework';

import '@fortawesome/fontawesome-free/js/brands';
import '@fortawesome/fontawesome-free/js/regular';

const w = (window as any);

@inject(UxIconMap)
export class App {

  constructor(private uxIconMap: UxIconMap) {
    if (w.___FONT_AWESOME___ && w.___FONT_AWESOME___.styles) {
      type FabIcon = [Number, Number, Array<any>, string, string];
      for (let fKey in (window as any).___FONT_AWESOME___.styles) {
        let fIcons: {[key: string]: FabIcon} = (window as any).___FONT_AWESOME___.styles[fKey];
        let fSet: Array<{name: string, material: string}> = [];
        for (let iconKey in fIcons) {
          const icon = fIcons[iconKey];
          fSet.push({name: `fab_${iconKey}`, material: `<svg xmlns="http://www.w3.org/2000/svg" viewBox="0 0 ${icon[0]} ${icon[1]}"><path d="${icon[4]}"/></svg>`});
        }
        this.uxIconMap.registerIcons(fSet);
      }
    }
  }

}

End result:

icons

I find it cool :-)

ben-girardet commented 4 years ago

@bigopon do you have any feedback/input regarding this PR and more specifically the import question for Icon Sets. Is it a good idea to have the icon sets as JSON files that we can import from the root directory ?

ben-girardet commented 4 years ago

I've quickly put together a little tool to generate Icon Sets (JSON, but could be anything once we agree on the format).

It currently generate sets from https://materialdesignicons.com/ (more sources can be added later).

See here: https://github.com/platform5/ux-icon-builder

EisenbergEffect commented 4 years ago

This makes sense and it seems there's some precedent from other projects as well.

ben-girardet commented 4 years ago

@EisenbergEffect @serifine @bigopon

This PR is ready for final review.

ben-girardet commented 4 years ago

@EisenbergEffect in fact I'm suggesting to go one step further. Based on my observation of FontAwesome JS icons, I discovered that they save them in an JSON Array like:

[
  [
    "icon-name",
    24, // view box width
    24, // view box height
    "..." // charCode
    "M19" // svg path
  ]
]

Because they also provide their sets in font format they also include a charCode value. In our case, because we are SVG based, we could do :

[
  [
    "icon-name",
    "M19", // svg path
    24, // optional view box width (default 24)
    24 // optional view box height (default 24)
  ]
]

With this we can reduce the JSON size a little by removing the SVG markup and only providing width/height for non-default sizes (such as when we import FontAwesome icons).

Thoughts ?

EisenbergEffect commented 4 years ago

@ben-girardet That sounds great. Let's do it. BTW Would you mind emailing me at rob at bluespire dot com ?

ben-girardet commented 4 years ago

This version do not automatically import any icons by default. From now on, you must decide on a per-project basis what icon sets you want to use.

Here are a few exemples on how you can prepare your icon sets:

1. Import full set when configuring the plugin

As a quick start, you can import the same icon sets that was previously shipped with the package by configuring the plugin as such in your main.ts

// main.ts

/* Add this line to import the full set into an `icons` variable */
import icons from '@aurelia-ux/icons/sets/full-array.min.json';

/* When you initilize the plugin, you can pass on this `icons` variable */
aurelia.use.plugin(PLATFORM.moduleName('@aurelia-ux/icons'), {icons: icons})

/* That's it ! */

2. Lazy loading later

// app.ts

import { inject } from 'aurelia-framework';
import { UxIconMap } from '@aurelia-ux/icons';
// here we import the full set from @aurelia-ux/icon but you can define any
// icon set in the proper format (see below)
import icons from '@aurelia-ux/icons/sets/full-array.min.json';

@inject(UxIconMap)
export class App {
  constructor(private iconMap: UxIconMap) {
    iconMap.registerIcons(icons);
  }
}

3. Import Font Awesome Icons

If you want to use Font Awesome icons, you can use them inside the <ux-icon> component. First you need to install the icons like:

npm install @fortawesome/fontawesome-free

Then you can import them like so:

// app.ts

import { inject } from 'aurelia-framework';
import { UxIconMap, UxIconRegArray } from '@aurelia-ux/icons';

// You can import any font awesome set like so.
// They will all become available in the ux-icon component
import '@fortawesome/fontawesome-free/js/brands';
import '@fortawesome/fontawesome-free/js/regular';

const w = (window as any);

@inject(UxIconMap)
export class App {
  constructor(private uxIconMap: UxIconMap) {
    if (w.___FONT_AWESOME___ && w.___FONT_AWESOME___.styles) {
      type FabIcon = [number, number, Array<any>, string, string];
      for (let fKey in (window as any).___FONT_AWESOME___.styles) {
        let fIcons: {[key: string]: FabIcon} = (window as any).___FONT_AWESOME___.styles[fKey];
        let fSet: Array<UxIconRegArray> = [];
        for (let iconKey in fIcons) {
          const icon = fIcons[iconKey];
          fSet.push([iconKey, `<path d="${icon[4]}"/></svg>`, icon[0], icon[1]]);
        }
        this.uxIconMap.registerIcons(fSet);
      }
    }
  }
}

Registered icon list

You can get the list of all registered icons by calling iconMap.getAllKeys() which will return a Array<string> with all icon names. Knowing this you could add the following code to your template to have a preview of all your registered icons with their names:

<ux-grid>
  <ux-card sm="2" repeat.for="name of uxIconMap.getAllKeys()">
    <ux-icon icon="${name}"></ux-icon>
    <br >${name}
  </ux-card>
</ux-grid>

Icon Set Format

You can register icons in an object or array form. The array form is slightly lighter.

// array form
[
  [
    "person", // icon name
    "<path d=\"M12 ...\"></path>", // svg path (without the svg tag)
    24, // optional (viewport width, default to 24)
    24 // optional (viewport height, default to 24)
  ],
  [
    "other-icon-name",
    "<path>...</path">
  ],
  ...
]
// object form
[
  {
    "name": "person", // icon name
    "svg": "<svg viewBox=\"0 0 24 24\"><path d=\"M12 ...\"></path></svg>" // svg path (with the svg tag)
  },
]

Importing JSON

Note for typescript users: make sure that your tsconfig.json is set to allow importing JSON files. This can be achieved by settings "resolveJsonModule": true in the compilerOptions section.

ben-girardet commented 4 years ago

@EisenbergEffect I've updated the PR with a class-level (configurable) property for defaultIconWidth and defaultIconHeight.

I didn't know what you meant by adding a section to the PR so I wrote a short note in my previous comment that could be added to the release note.

I think this PR is ready for merge.

bigopon commented 4 years ago

Nice! I think we may also want to add another small example of how to fetch it dynamically before initializing the icon package, or after initialization as well. Folks may have valid reasons to do it lazily.

ben-girardet commented 4 years ago

Good idea @bigopon I've added more exemples by editing my comment above.

bigopon commented 4 years ago

@ben-girardet in the step 3, where does ___FONT_AWESOME___ come from? And thanks for the comment, I think this will be a pretty good baseline for later doc improvement related to icons

ben-girardet commented 4 years ago

@ben-girardet in the step 3, where does ___FONT_AWESOME___ come from? And thanks for the comment, I think this will be a pretty good baseline for later doc improvement related to icons

good catch, forgot the import part of the Font Awesome packages. Updated my comment, and added a section to explain how to get and see the registered icons.

ben-girardet commented 4 years ago

@EisenbergEffect I've rebased my branch and merge with this PR. I've added some lint fixes.

@bigopon do you know if the npm run test or npm run doc are supposed to work? I've tried to run them and get quite a few errors that I struggle to fix. I'm trying to understand if I broke something or if these tests and docs were already not working before hand ?

bigopon commented 4 years ago

@ben-girardet for now, both of them may not work. If it's not blocking you, give me a few days and i'll resolve it to prepare for a more solid infra with regards to those. If you have any ideas, can you put it here for now?

ben-girardet commented 4 years ago

@bigopon thanks. It's not blocking for now and I believe we can merge this PR as this.

In regards to tests and docs I must say that I'm not (yet) familiar with setting this up. Very looking forward to see how you can set it up for this lib and learn from then.

EisenbergEffect commented 4 years ago

@ben-girardet With the merge of this PR, do you think we're ready for a release?

ben-girardet commented 4 years ago

@EisenbergEffect I've tried to clone, build and run the dev app and all works fine.

My last question regards /dist files. In the PR I've only pushed the /src. Are you building the dist before the release or not ? Just asking as I'm very new with the process.

ben-girardet commented 4 years ago

Just found a small bug in the icon package. Will submit a new PR with the fix in a moment...

EisenbergEffect commented 4 years ago

Got the bug fix PR in. Regarding the dist folder, I'll build that as part of the release process. With that in mind, anything else we need to do before I release or are we ready to go?

ben-girardet commented 4 years ago

Ready to go ! Thanks @EisenbergEffect