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

[Icons] allow configuring multiple local directories and optionally prefixing them #1596

Open
wants to merge 2 commits into
base: 2.x
Choose a base branch
from

Conversation

kbond
Copy link
Member

@kbond kbond commented Mar 9, 2024

Q A
Bug fix? yes
New feature? yes
Issues n/a
License MIT

Configure multiple paths:

ux_icons:
    icon_dir:
        - %kernel.project_dir%/assets/icons
        - %kernel.project_dir%/vendor/your-vendor/your-bundle/assets/icons

Additionally, you can suffix the path with @<alias> to add a prefix for icons in this directory:

ux_icons:
    icon_dir:
        # ...
        - %kernel.project_dir%/vendor/fortawesome/font-awesome/svgs@fa

Now you can render icons in this directory by prefixing the name with fa::

{{ ux_icon('fa:solid:adjust', {class: 'w-4 h-4'}) }} <!-- renders "vendor/fortawesome/font-awesome/svgs/solid/adjust.svg" -->

@carsonbot carsonbot added the Status: Needs Review Needs to be reviewed label Mar 9, 2024
@smnandre
Copy link
Collaborator

smnandre commented Mar 9, 2024

I need something similar for the rendering options... in the end we maybe will need to merge the syntax into something a bit more complex, lire the transports, or the doctrine connections.

I guess thoses are "read-only" ? (as we won't be able to write in "vendors" with the import command)

->scalarNode('icon_dir')
->info('The local directory where icons are stored.')
->defaultValue('%kernel.project_dir%/assets/icons')
->arrayNode('icon_dir')
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should I rename this to icons_dirs?

Copy link
Member

@yceruto yceruto Mar 9, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

maybe paths if they can be relative?

@kbond
Copy link
Member Author

kbond commented Mar 9, 2024

I need something similar for the rendering options... in the end we maybe will need to merge the syntax into something a bit more complex, lire the transports, or the doctrine connections.

Something like?

ux_icons:
  icon_dir:
    - path: some/path
      prefix: fa
      readonly: true
      more: ...

I guess thoses are "read-only" ?

All but the first one, yeah. We could make configurable as shown above.

@yceruto
Copy link
Member

yceruto commented Mar 9, 2024

ux_icons:
    paths:
        vendor/fortawesome/font-awesome/svgs: fa

? ... then normalize to path, prefix?

@yceruto
Copy link
Member

yceruto commented Mar 9, 2024

Also, it seems the concept is pretty close to Twig namespaces. It could be good if we reuse same concept that we already know, so "prefix" could be named "namespace" and the usage could ux_icon('@fa/solid/adjust.svg'), it's a path to a resource in the end, right? Less new magic/convention, more easy to understand.

@smnandre
Copy link
Collaborator

smnandre commented Mar 9, 2024

ux_icons:
    paths:
        vendor/fortawesome/font-awesome/svgs: fa

I'd use the "prefix" as key, like a namespace. As it's the only thing we need to garantee the unicity.

So maybe

    dfgdfsdfd:    # word i don't have; set ? packs ? groups ? ns ? 
        bi:
            path: assets/bootstrap
       ld: 
           alias: lucide
      bi-reg:
            dir: vendor/fortawesome/font-awesome/svgs:

Do you agree to make it the "primary key" ?

@kbond
Copy link
Member Author

kbond commented Mar 9, 2024

Yeah, prefixing with a key seems best - this will allow choosing the path to import into when using the import command. And @yceruto, namespacing is an interesting idea. Maybe something like:

ux_icons:
  paths:
    default: some/path

    fa:
        path: vendor/some/path
        readonly: true
        namespace: fa

About the : separator. Twig switched from : to / eventually, I wonder if we should do the same here initially (have no concept of :). Like @yceruto says, keep to twig's conventions.

@smnandre
Copy link
Collaborator

smnandre commented Mar 9, 2024

I'd say...

  • prefix/key: no "directory" allowed (no ":", no "/")
  • path: free as a french mobile phone carrier

Regarding "name/identifier":

  • HTML syntax uses the ":" notation, i'd feel maybe more logic to keep it... but if you want to use "/" i'm not gonna fight for this :))
  • Twig component (in component function) names: i'll check, but i think we have 0 constraints there, so no hot take there either.

@smnandre
Copy link
Collaborator

How can we move on this ? What do you need @kbond ? Or can maybe start in parralel the attributes configuration per directory and we see later how to merge ?

@kbond
Copy link
Member Author

kbond commented Apr 1, 2024

prefix/key: no "directory" allowed (no ":", no "/")

I think we need to allow a directory even when a prefix is used. For instance, assets/icons/heroicons: hero could have hero:24:outline:user/hero:24:solid:user

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Status: Needs Review Needs to be reviewed
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants