waterplea commented 3 years ago

🚀 Feature request

We need to add a guide on how to use different icon set. Perhaps a whole section on customisation with instructions on different aspects, such as theme, custom dialogs, etc.

fynnfeldpausch commented 3 years ago

I guess that a bit of refactoring is also needed. While I managed to change the icon set, most of the icon names are hard coded in the components. A new iconset does not necessarily use the same names. So they would have to be configurable.

waterplea commented 3 years ago

@fynnfeldpausch what would you suggest? In theory, you can change it even now, with hardcoded names. Since TUI_ICONS_PATH is a handler where you can provide mapping from Taiga UI icon names to your custom icons. I realize it's a bit clumsy so I'd like to hear what you have in mind. Also Taiga UI icons are processed in a way to allow using them through USE tag to inline from assets. There's a script that one can run over their icons set to work the same, or icons can be bundled with the app by providing them with TUI_ICONS. Using icons from assets have the benefit of smaller bundle app while bundled icons load immediately.

fynnfeldpausch commented 3 years ago

Good question. The only idea I can come up with right now is to use the same DI mechanism we discussed in another ticket and make the icons configurable via a provider.

By the way, is there a reason you are keeping width and height values in the SVG code of your icons? If you would remove it and just use the view box definition, you could scale your icons. Just a thought...

waterplea commented 3 years ago

This is by design because our internal icons are required to be used at 16 and 24, as they are initially sized. Also I don't think it's possible to scale "named" icons that are inlined through USE tag. In MarkerIcon component we get around it by using transforms: https://taiga-ui.dev/tui-marker-icon Even bundled icons end up used by name and inlined with USE to avoid extra DOM manipulations. It is actually a significant performance boost over setting innerHTML with SVG code if you have many components with icons on the page.

fynnfeldpausch commented 3 years ago

@waterplea Unfortunately I cannot provide a stackblitz as it does not support assets, but I just checked it locally and the SVGs seem to scale without any problems when the explicit sizing is removed. Tested it with the following SVG code for tuiIconLikeLarge.svg (which can be seen on the SVG docs page):

<svg xmlns="http://www.w3.org/2000/svg" focusable="false"><g id="tuiIconLikeLarge" xmlns="http://www.w3.org/2000/svg"><svg viewBox="0 0 24 24" fill="none" xmlns="http://www.w3.org/2000/svg"><path d="M7 22H4a2 2 0 0 1-2-2v-7a2 2 0 0 1 2-2h3m7-2V5a3 3 0 0 0-3-3l-4 9v11h11.28a2 2 0 0 0 2-1.7l1.38-9a1.999 1.999 0 0 0-2-2.3H14z" stroke="currentColor" stroke-width="2" stroke-linecap="round" stroke-linejoin="round"/></svg></g></svg>

Scaling the icons via width and height on the tui-svg element.

waterplea commented 3 years ago

Are you passing source code directly to tui-svg? If you use name and not the source code it would go through USE tag. There's no way for shadow dom inside it to tell the size of parent so you cannot scale it like that.

fynnfeldpausch commented 3 years ago

Don't quite get your question. What I did was to start the project locally and just changed the content of the icon SVG file. The icon is still loaded as before and will occupy 100% space of the surrounding svg component, which then can be scaled.

waterplea commented 3 years ago

You're right. Looks like my memory failed me and the only reason size is there is to enforce it on proprietary icons to follow design guidelines. Seems like we can drop the hardcoded size on the open source package. This could break some existing styles in components though, so we need to be careful.

waterplea commented 3 years ago

On the other hand, we do not want those icons to expand if they are used in larger buttons, however we might want to use larger icons in those so we do not want to enforce 24px by 24px on the wrapper. I have a feeling it might be better to leave it as is. Material icons also have sizes hardcoded when you download them. Anyway, I have added a section on custom icons and I will go over it this Friday. I will close this issue with a link to customization video then. https://taiga-ui.dev/icon-set

fynnfeldpausch commented 3 years ago

@waterplea Yeah I understand that. And I am also fine with not changing it. I just think it is worth consideration due to the following reasons:

SVGs are scalable vector graphics after all, so it feels weird to me to give that up
scaling via CSS transform is not only more expensive (although I think it does not make a difference in modern browsers) but as far as I know it is also just a "zoom" so basically a lossy conversion
it is quite unintuitive to have to override two icons for a single component, just because it switches the entire icon for a smaller size
I implemented the changes and was able to get rid of a lot of icons (as they are just duplicates) and also reduce the size of the existing ones. The size of the icons was almost reduced by 50%
it limits the way the icons can be customized. You are kind of expecting other iconsets to also define the size of the icons within the SVGs. If an iconset does not do that and goes for a scalable implementation, the icons will end up in the wrong size.

I have referenced a PR - not necessarily ready for merge but it sure does give an impression on how that would look like and what kind of changes would have to be made. I guess there are still a couple of smaller issues open in that PR.

Also worth pointing out that some icons have not the right size at the moment due to a wrong viewbox. But as I said, Its more a showing of the WIP.

waterplea commented 3 years ago

Worth pointing out that small icons are not just smaller versions of big ones. They are less detailed, retaining the same line width. It is important for consistent interface.

waterplea commented 3 years ago

Here's a session on customization: https://www.youtube.com/watch?v=JK0Fp7bQ52w @fynnfeldpausch if you could start another discussion on icons and DI that you mentioned, please do. Feel free to copy any arguments from your messages above if you think they are relevant.

0xphilipp commented 3 years ago

I still have some issues with this topic. I try to place ClarityIcons into the TUI_ICONS Injection Token like this:

    {
      provide: TUI_SANITIZER,
      useValue: <Sanitizer>{
        sanitize: (contxt, inp) => inp,
      },
    },
    {
      provide: TUI_ICONS,
      useValue: {
        tuiIconHideLarge: clarityIcons.get('eye-hide'),
        tuiIconShowLarge: clarityIcons.get('eye'),
      },
    },

But then in this line, it returns an empty string instead of the icon. Both isShadowDom and isName return false:

The example icon is this:

<svg version="1.1" class="has-solid " viewBox="0 0 36 36" preserveAspectRatio="xMidYMid meet"
    xmlns="http://www.w3.org/2000/svg" xmlns:xlink="http://www.w3.org/1999/xlink" focusable="false" role="img"><path d="M33.62,17.53c-3.37-6.23-9.28-10-15.82-10S5.34,11.3,2,17.53L1.72,18l.26.48c3.37,6.23,9.28,10,15.82,10s12.46-3.72,15.82-10l.26-.48ZM17.8,26.43C12.17,26.43,7,23.29,4,18c3-5.29,8.17-8.43,13.8-8.43S28.54,12.72,31.59,18C28.54,23.29,23.42,26.43,17.8,26.43Z" class="clr-i-outline clr-i-outline-path-1"/>
            <path d="M18.09,11.17A6.86,6.86,0,1,0,25,18,6.86,6.86,0,0,0,18.09,11.17Zm0,11.72A4.86,4.86,0,1,1,23,18,4.87,4.87,0,0,1,18.09,22.89Z" class="clr-i-outline clr-i-outline-path-2"/>
            <path d="M33.62,17.53c-3.37-6.23-9.28-10-15.82-10S5.34,11.3,2,17.53L1.72,18l.26.48c3.37,6.23,9.28,10,15.82,10s12.46-3.72,15.82-10l.26-.48ZM17.8,26.43C12.17,26.43,7,23.29,4,18c3-5.29,8.17-8.43,13.8-8.43S28.54,12.72,31.59,18C28.54,23.29,23.42,26.43,17.8,26.43Z" class="clr-i-solid clr-i-solid-path-1"/>
            <circle cx="18.09" cy="18.03" r="6.86" class="clr-i-solid clr-i-solid-path-2"/></svg>

Usage is from the password input:

waterplea commented 3 years ago

Seems like this is expected. Like the comment says, it is supposed to return empty line for innerHTML and add icon by name through USE tag. Can you provide a reproduction either as Github repo or StackBlitz so I can debug it? The issue is someplace else, either icons was not processed properly by our SVG service or there is something wrong with SVG component.

0xphilipp commented 3 years ago

Here is an example: https://stackblitz.com/edit/taiga-starter-18vvcb?file=src/app/app.module.ts I think there is an issue in SVG component. When overriding the svg with TUI_ICONS then the svg-component needs to show that instead of using shadow dom ref.

waterplea commented 3 years ago

@Phmager thank you for reproduction. It always uses Shadow DOM when you work with icons by name, however I found an issue with the service — it assumes all icons with names starting with tuiIcon are already processed and skips required step to get it to work. I'll fix that soon. One other thing is, our icons use color CSS style to control icons color but this pack expects fill rule. We had fill set to transparent by default because of our proprietary icons sometimes have optional second color which you control with fill. I will change fill: transparent to fill: currentColor in public code so this works for you and other similar cases.

0xphilipp commented 3 years ago

great 👍

0xphilipp commented 3 years ago

Here is how it will work, if anyone wants to use both tui and other icons:

let clarityIcons = (window)['ClarityIcons'];

let overridden = {
  tuiIconHideLarge: clarityIcons.get('eye-hide'),
  tuiIconShowLarge: clarityIcons.get('eye'),
  tuiIconCheck,
};

let overriddenIconKeys = Object.keys(overridden);
let iconsPath = iconsPathFactory('assets/icons/');

{
  provide: TUI_ICONS_PATH,
  useValue: (val: string) => (overriddenIconKeys.includes(val) ? '#' + val : iconsPath(val)),
},
{
  provide: TUI_ICONS,
  useValue: overridden,
},

Doc-Code commented 3 years ago

Это сделано намеренно, потому что наши внутренние значки должны использоваться в значениях 16 и 24, поскольку они изначально имеют размер. Также я не думаю, что возможно масштабировать «именованные» значки, встроенные в USEтег. В компоненте MarkerIcon мы обходим это с помощью преобразований: https://taiga-ui.dev/tui-marker-icon Даже связанные значки в конечном итоге используются по имени и встраиваются, USEчтобы избежать дополнительных манипуляций с DOM. На самом деле это значительный прирост производительности по сравнению с настройкой innerHTML с помощью кода SVG, если у вас есть много компонентов со значками на странице.

You can also specify the viewBox and change the size of the icons.

taiga-family / taiga-ui

[FEATURE] Using other icons #107

🚀 Feature request