Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Custom icons must be in kebab case #265

Open
huang-julien opened this issue Oct 1, 2024 · 19 comments
Open

Custom icons must be in kebab case #265

huang-julien opened this issue Oct 1, 2024 · 19 comments
Labels

Comments

@huang-julien
Copy link
Member

huang-julien commented Oct 1, 2024

Description

iconify's getIcon uses validateIconName which enforce some validation for an icon's name.

One validation is that a name must be split with - which is often not the case with users custom icons

Reproduction

https://stackblitz.com/edit/nuxt-starter-qlykyd?file=icons%2FEuro.svg,nuxt.config.ts,app.vue

in this reproduction, we can't see my-icons:Euro because Euro is in PascalCase

Solution

Either a documentation fix about naming convention or fix it in iconify or we could also merge prefix with the name ?

@YaredFall
Copy link

YaredFall commented Oct 9, 2024

Stumbled upon this quirk too.

It seems that only lowercase letters, digits and dash (-) is allowed inside icon filename.

I was hoping to be able to use uppercase letters and underscores... I wonder why this naming convention is never mentioned in the docs.

@huang-julien
Copy link
Member Author

Because we're using @iconify and this is the convention @iconify uses

@huang-julien
Copy link
Member Author

I'll work on it when i have some free time

@polarove
Copy link

Same here, tortured me for an hour

@JonathanXDR
Copy link

JonathanXDR commented Oct 27, 2024

Would it also be possible to consider supporting dots (.) as separators for custom icons? This would add flexibility to naming conventions and align more closely with custom design systems, as, for example, it’s a common pattern at the company I work for.

@antfu
Copy link
Member

antfu commented Oct 28, 2024

/cc @cyberalien, do you see this possibly being supported by Iconify, or what do you suggest we do with a custom solution for it? Thanks a lot!

@antfu antfu added the upstream label Oct 28, 2024
@antfu
Copy link
Member

antfu commented Oct 28, 2024

Duplicated as #257

@cyberalien
Copy link

Naming convention is used in all parts of Iconify ecosystem, so changing it would be tricky. It would require way too many code changes.

I recommend changing it internally in Nuxt Icon. Convert names when loading and when passing icon name parameter. It should be as simple as changing icon name to lower case.

@antfu
Copy link
Member

antfu commented Oct 28, 2024

I added automatical convention and warnings upon loading to make this more transparent - as a temporary workaround. (c09fdc7, v0.6.1)

In the long term, I do wish to support more relaxed conversion (at least _ and .) - which I think might require us to wrapping or hack the iconify runtime a bit to do that.

@cyberalien
Copy link

In the long term, I do wish to support more relaxed conversion (at least _ and .) - which I think might require us to wrapping or hack the iconify runtime a bit to do that.

I think support for any icon names can be added in future versions. Current convention will remain for icons loaded from API, but can be ignored for custom icons.

@antfu
Copy link
Member

antfu commented Oct 28, 2024

I would agree that the Iconify core should be kept limited. But would you consider that for custom provider/API endpoint, such custom names could still be useful to match with their custom conversion etc.? Maybe allow the provider to define a charset that is considered to be the valid name?

In a way Nuxt Icon has a server API endpoint that serves as a custom provider to serve the icon data in our app and fallback to Iconify's public API. This is also where we put custom icon data in. For some cases the custom icons could be bundled directly in the client via addIcon() API, but for the other cases, it might be served from a normal Iconify-liked endpoint.

(and this is a slide of my upcoming talk, for making it clear)
Image

@cyberalien
Copy link

cyberalien commented Oct 28, 2024

It is not simple. Icons are not loaded one by one, they are loaded in bulk from API, otherwise loading could take a while. So API end point is not easily customisable.

I think a simple solution would be to allow custom loaded based on prefix and/or provider. Then current API logic won't need to change, but it would allow custom solutions, which could be used to load icons from Nuxt. Then naming convention would apply only to icons loaded with default loader.

@antfu
Copy link
Member

antfu commented Oct 28, 2024

I think a simple solution would be to allow custom loaded based on prefix and/or provider.

Oh yeah, I think that would be great to serve our needs!

@cyberalien
Copy link

I'm thinking of making 3 ways to handle loading:

  • setIconSetLoader(callback, prefix, provider = '') - loader for an entire icon set, could be useful if loading whole icon set at once.
  • setIconsLoader(callback, prefix, provider = '') - loader for multiple icons, could be useful to fetch multiple icons in one request.
  • setIconLoader(callback, prefix, provider = '') - loader for one icon, could be useful when fetching only one icon at a time.

Types for callbacks to make it clearer how callbacks could be implemented:

type IconSetLoaderCallback = (prefix: string, provider: string) => Promise<IconifyJSON | null>;
type IconsLoaderCallback = (names: string[], prefix: string, provider: string) => Promise<IconifyJSON | null>;
type IconLoaderCallback = (name: string, prefix: string, provider: string) => Promise<IconifyIcon | null>;

Idea is to set at least one method, icon component will use whatever is available:

  • if loading whole set is possible, use that
  • if loading multiple icons is possible, use that
  • otherwise load each icon one at a time

Parameters in function and callback are ordered from most useful first to least useful last. All loaders are async functions that return Promise instance. Result is data in correct type, null if icon is missing.

What do you think?

@antfu
Copy link
Member

antfu commented Oct 28, 2024

Personally, I think that setIconsLoader would be enough. setIconLoader would be equivalent to setIconsLoader with a single-element array. While setIconSetLoader, I am not so sure how useful people would want the whole set (+ in those cases they can load that manually with addCollection etc).

Even if, in this case, the user endpoint only supports loading a single icon, on the loader side, the user could still dispatch multiple requests and merge the results into one.

Having only one API would also be made it easy on the Iconify side, I think.

@cyberalien
Copy link

Working on solution: iconify/iconify#348

@cyberalien
Copy link

Added 2 loaders:

  • For multiple icons
  • For one icon

You can use one or both. If both are set, loader for one icon is called when loading only one icon, otherwise loader for multiple icons is called.

Loaders have same signature, but different callbacks:

setCustomIconsLoader(callback, prefix, provider = '')
setCustomIconLoader(callback, prefix, provider = '')

where callback is possibly async function that returns data or null.

Data is IconifyJSON icon set for setCustomIconsLoader, IconifyIcon icon for setCustomIconLoader.

Callback can be asynchronous or synchronous. Types for callbacks:

/**
 * Custom icons loader
 */
export type IconifyCustomIconsLoader = (
	icons: string[],
	prefix: string,
	provider: string
) => Promise<IconifyJSON | null> | IconifyJSON | null;

/**
 * Custom loader for one icon
 */
export type IconifyCustomIconLoader = (
	name: string,
	prefix: string,
	provider: string
) => Promise<IconifyIcon | null> | IconifyIcon | null;

Icon or list of icons is first parameter because other parameters are most likely will not be used, unless you are reusing same loader for multiple prefixes.

Why 2 loaders? Loader for one icon can be used as solution for customising icons: set loader for a custom prefix, use it to load icon from another icon set (possibly from API) using loadIcon function, modify response to change stuff like colors or stroke width, return modified icon:

setCustomIconLoader(async (name) => {
	const data = await loadIcon(`tabler:${name}`);
	return data
		? {
				...data,
				body: data.body.replaceAll(
					'stroke-width="1.5"',
					'stroke-width="3"'
				),
		  }
		: null;
}, 'tabler-fat');

For now implemented in core, I'll add it to all components later today.

Loaders work with custom prefixes and icon names that have only one requirement: not being empty strings. So you can use whatever you want, even filenames (though : might cause problems, so strip that and trim string... basically do a simple sanity check in names to avoid surprises).

@cyberalien
Copy link

Published @iconify/vue version 4.2.0-dev.1 with new loader. Install as @iconify/vue@next.

@cyberalien
Copy link

cyberalien commented Nov 3, 2024

Notes about naming requirements.

Icon still must have a prefix and name parts, separated with :. Separator cannot be changed and is required. Changing that behavior would require way too many changes, which is not feasible.

Names like my-icons:Euro now work correctly. Since other characters are not limited, for emulating file system stuff you can use my-icons:svg/whatever.svg, as long as there is only one : in name and prefix is set.

Entities and unicode are not parsed. Everything is kept as is. So be careful when using non-latin characters that might end up being encoded differently.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

6 participants