Open ypnos-web opened 6 years ago
That's an interesting idea, maybe a better idea for the templates would be to append something to 'icon'
, e.g.:
'icon' => '<i class="fa-{{type}}{{attrs.class}}"{{attrs}}></i>',
'icon-s' => '<i class="fas fa-{{type}}{{attrs.class}}"{{attrs}}></i>',
'icon-b' => '<i class="fab fa-{{type}}{{attrs.class}}"{{attrs}}></i>'
And when parsing the easy-icon, you get i:s:<iconame>
and you render using icon-s
. This way, users can easily add the icon templates they want and have them working with easy icons.
The following regex should work for parsing the icon:
(^|\s+)i:([a-zA-Z0-9\\-_]+:)?([a-zA-Z0-9\\-_]+)(\s+|$)
Something could be achieved by backing the icon
template prior to the regex parsing, but maybe it is a better idea to add a custom option to the icon
method in the HTML helper, like adding a 'template'
option and defaulting it to 'icon'
? Or maybe if you have some ideas on how the easy-icon trait can be detached from the HTML helper, I'll be glad to hear them, since it's something I am not really happy with...
I agree that the entry point to this functionality would be HtmlHelper's icon()
method that could take a custom option for it. I will also have a closer look at the easy icon trait to see if I find a way to improve how it interfaces with the rest.
Options I considered:
icon()
a functionality provided by EasyIconTrait
and use the trait in HtmlHelper
to expose to the user. All functionality is in one place. Drawback: Configuring the template does not work as expected (the template would need to be configured for all helpers exhibiting the trait individually).EasyIconTrait
to HtmlHelper
. All functionality, again, is in one place. Drawback: _makeIcon()
and _easyIcon()
need to be public methods and their hacky interface will be exposed.IconHelper
for this. Drawbacks: Overkill, same drawback as option 2.Have you thought about reversing how easyIcon works to make the interface less ugly? Consider replacing the call:
$this->_easyIcon('parent::link', 0, 2, [$title, $url, $options]);
with:
$this->Html->injectIcon($title, parent::link($title, $url, $options));
Now what insertIcon
does is regex match $title within the already formatted HTML output of parent::link()
and replace the icon at this point. No option mangling is needed.
Signature:
public function injectIcon($title, $text = null, $options = []);
Advantages:
_makeIcon()
)What do you think about this idea?
This sounds interesting, I'd rather have this functionality in a IconHelper
to separate it from HtmlHelper
. HtmlHelper::icon
could then falls back to IconHelper::icon
(or something similar) for simplicity/compatibility.
You'd need to set templates for IconHelper
instead of HtmlHelper
but I think it's an improvement rather than anything else (HtmlHelper
already contains two much templates... ).
I think we would need something to disable easy icon if needed, maybe:
$this->Icon->inject = false; // another name maybe
But it would be quite difficult to separate "automatic" injection from manual one... Maybe something else than a simple switch? I don't mind if this is not as simple as switching a variable to true/false since this is probably not used that often, I'd rather have something clean for this (if we make the icons clean, why not make everything clean ? 😃).
The regex for the pattern would need to be changed since it actually only matches beginning of string / pattern between spaces. At first glance this does not look too complex (would simply need to match >
followed by spaces), but this would require more extensive testing than what is actually available.
Regarding the regex, my idea was to apply it on $title and then do a str_replace
of $title within $text (if $title had matched and was actually altered). This is why it takes $title as a separate parameter, to make it safer against false replacements outside the title. We could harden it further by enforcing sth like '>{$title}<'.
I am not fan of this idea because:
str_replace
against the possibly escaped $title
, this means more work for the caller (need to pass $options
) and the method (need to check), and it makes the interface not so user-friendly (people have to know that they should pass the escape
if necessary).$title
before sending it to injectIcon
, but this would require more work from the caller (in particular for all the automatically injected icon, this would add lots of boilerplate code... ).$this->Icon->injectIcon('i:home', $this->SomeHelper->someMethod('i:home'));
I'd rather remove the $title
option and parse the final $text
, I don't think there would be much problem with this, especially if we provide something to disable it when needed, or maybe better, allow user to customize the matching pattern (and eventually disable it).
So the two issues you raise are argument duplication and how to deal with escaping.
For me the argument duplication is not a big deal as I only see calls for this method from other helpers or custom widget classes like one I wrote for myself. The usual developer will just call these or use icon()
directly. So only few places in library/plugin code call injectIcon
.
Escaping issue is a valid point, however I believe we would get away with simply trying to replace both escaped and unescaped title (try unescaped first, if it fails, escape it and try again). So it works transparently from the outside and in almost all cases, the first test will be the end of it.
My counter argument for parsing the whole output is that people might have weird tag options where an 'i:something' slips in (e.g. data attributes). A test for '>i:something '
|| ' i:something<'
would certainly migitate this problem but would fail to work with a (stupid) button template like '<button>%nbsp;{{title}} </button>'
.
I am just putting out my thoughts here, I am happy to implement either approach, just name it.
Regarding an easy way to disable it, obviously we can have enableInjection()
/ disableInjection()
methods. I would like an easy way to do it case-by-case, e.g.
$this->Form->button('i:1 j:2', ['icon' => false]);
but this would lead to quite some extra work for the FormHelper (take icon option from option array, unset it in option array, pass it to injectIcon…), so it's probably not what we want after all.
I am answering the last part of the comment first — I agree it would be great to have a way to disable the injection case-by-case, even though I think icon
is too generic here, maybe inject-icon
😀
I think there is a simple way to disable some options from being parsed as an attribute: we need to override formatAttribute
inside the template class, but since helpers are already using a custom template class, this should not be a lot of work. We could then simply forward all options from the FormHelper
to the IconHelper
, e.g.:
$this->Icon->injectIcon($title, parent::link($title, $url, $options),
['_original' => $options]);
Using a custom (documented) option name so that other options (e.g. class
or id
) are not used for the actual icon
.
This _original
option could also be used for the escape
if we choose to apply the pattern only on $title
, even if this is less easy to do since the default escape
is not the same for all methods...
Regarding the parsing now, I agree that this may raise some issue, but as I said, I think we could have a "standard" rule, e.g. very generic, and then user would disable the injection when needed. Something very generic could be:
(^|[^\p{L}\p{N}]+)i:([a-zA-Z0-9\\-_]+:)?([a-zA-Z0-9\\-_]+)([^\p{L}\p{N}]+|$)
We could also allow user to set a specific pattern if they want (documenting it correcty), so they could do:
// Current one:
$this->Icon->setMatchingPattern('(^|[\w\W\d]+)i:([a-zA-Z0-9\\-_]+:)?([a-zA-Z0-9\\-_]+)([\w\W\d]+|$)');
// New one?
$this->Icon->setMatchingPattern('(^|[^\p{L}\p{N}]+)i:([a-zA-Z0-9\\-_]+:)?([a-zA-Z0-9\\-_]+)([^\p{L}p{N}]+|$)');
// Disable?
$this->Icon->setMatchingPattern(false);
// Enable? Falls back to the default one?
$this->Icon->setMatchingPattern(true);
Great idea of allowing matching pattern customization, I would add that I think for the user it is cleaner to have a setMatchingPattern() / enableMatching() / disableMatching() interface, as it is self-explanatory.
Yes, it would probably be better, I have to check CakePHP's convention for this if there are any (maybe setMatchingEnable(true / false);
.
Just checked, it is enableMatching
, disableMatching
.
I think we could use named capture group in the regex so that user can customize how template are chosen depending on the pattern, e.g.:
// The <type> group is the special one used to select the actual icon.
$pattern = (^|[^\p{L}\p{N}]+)i:(?<type>[a-zA-Z0-9\\-_]+)([^\p{L}\p{N}]+|$);
$templatePattern = 'icon';
// New pattern, {{shape}} will be replaced by the matching:
$pattern = (^|[^\p{L}\p{N}]+)i:((?<shape>[srb]):)?(?<type>[a-zA-Z0-9\\-_]+)([^\p{L}\p{N}]+|$);
$templatePattern = 'icon-{{shape}}';
The method setMatchingPattern
would take an optional argument modifying the template pattern (that would be 'icon'
or 'icon-{{shape}}'
by default).
This way, this is extremely customizable, I could even use something like:
$pattern = (^|[^\p{L}\p{N}]+)i((?<toolkit>[srb]):)?:((?<shape>[srb]):)?(?<type>[a-zA-Z0-9\\-_]+)([^\p{L}\p{N}]+|$);
$templatePattern = 'icon-{{toolkit}}-{{shape}}';
To switch between FontAwesome and e.g. Glyphicon simply by doing i:fa:home
or i:gl:home
.
Maybe we would need to find a way to find the template pattern when one of the matching group is not found, but that's something to consider I think. One idea would be to force a leading hyphen -
, e.g. the template pattern would be. icon{{toolkit}}{{shape}}
and {{toolkit}}
would be replaced by -$toolkit
only if tookit
is found by the matching.
I created a icon-helper
branch, it'll be easier for me to accept PR to this branch and then merge it to master
when everything is working as intended.
So this sounds like a version of preg_replace
that supports named back references. Do a preg_replace_callback
whereas the callback will fill-in all named captures as provided?
So in summary:
<template>
capture group is used to determine the template from above, or if not present, default is used.<shape>
(or <name>
?) capture group for the icon nameinjectIcon
option that is fished from '_original' or maybe '_parent' option array to disable functionality when falseicon()
from HtmlHelper and add a template option to it for convenience. Any other variables that the user defined in their templates they can provide with templateVars option.Outside IconHelper:
formatAttribute()
needs to strip/ignore injectIcon
in custom template class$options
as ['_parent' => $options]
icon()
method by IconHelperIf you agree with this I will start working on the code.
Just as a remark I think the possibility of translating regex captures to template variables might be a bit of an obscure feature for this specific context.
So this sounds like a version of preg_replace that supports named back references. Do a preg_replace_callback whereas the callback will fill-in all named captures as provided?
I think preg_replace_callback
handles named pattern since PHP 5.3, so it should be ok.
- We have several templates and a default template selection as configurable by the user. we can provide common templates (for glyphicon and fontawesome) as defaults and glyphicon would be the default template selection
Yes, and templates would be in the IconHelper
class.
- The user can configure the matching pattern, and the
<template>
capture group is used to determine the template from above, or if not present, default is used.
You mean have <template>
named groupe and use it like i:fa-icon-far:home
?
- The user can also use/reference to other capture groups in both matching pattern and template strings at will which will be generically filled. The default pattern and templates would include a
(or ?) capture group for the icon name
Yes, let's start with something matching shape
and type
(name to be confirmed) and replace it in a icon{{shape}}
pattern for template.
- There is a
injectIcon
option that is fished from '_original' or maybe '_parent' option array to disable functionality when false
Yes, I don't know if injectIcon
or inject-icon
would be the better, I think I have used the latter convention for some other options...
- We move over
icon()
from HtmlHelper and add a template option to it for convenience. Any other variables that the user defined in their templates they can provide with templateVars option.
Yes, HtmlHelper::icon
simply forwards everything to IconHelper::icon
(or some other method).
formatAttribute()
needs to strip/ignoreinjectIcon
in custom template class
Yes, this can be easily done by calling parent::formatAttribute(..., ['injectIcon']);
.
- all helpers use IconHelper for injecting icons, passing
$options
as['_parent' => $options]
Yes.
- HtmlHelper wraps
icon()
method by IconHelper
Yes.
Just as a remark I think the possibility of translating regex captures to template variables might be a bit of an obscure feature for this specific context.
Maybe, but you'd be surprised by the number of obscure feature in this plugin 😀 I don't think that much people are even aware of the easy icon feature. As long as this does not disturb the basic usage of this feature, I'm fine with adding this.
Ok, so I will be working on this.
@ypnos-web After closing the above-mentioned PR, I was thinking about a nice feature that we could implement using __invoke
and __call
. Below is a (non-working) example:
class IconHelper {
public function icon($type, $options = [], /* Extra? */) {
$options += [
'template' => $this->getConfig('defaultTemplate') // Something like this?
];
// Whatever...
}
public function __invoke() {
return call_user_func_array([$this, 'icon'], func_get_args());
}
public function __call($name, $arguments) {
// Function name -> Template
$result = $this->nameToTemplate($name);
// Update arguments...
$arguments[1]['template'] = $result;
return call_user_func_array([$this, 'icon'], $arguments);
}
};
Assuming $this->Icon
is the icon helper, we could have stuff like this:
$this->Icon->icon('home'); // Default icon
$this->Icon('home'); // Same as above, less verbose.
$this->Icon->fa('home'); // Use icon-fa template?
$this->Icon->faS('home');; // Use icon-fa-s template?
At first, I thought about using faIcon
as before, but since the helper is dedicated to icon, this seems redundant, and $this->Icon->fa('home')
seems better for me.
This is a (multiple allowed):
Since Fontawesome 5 got released, I am faced with the distinction between icon shapes 'solid', 'regular', 'light', and 'brand'. These are selected through a supplemental class 'fas', 'far', 'fab' instead of simply 'fa' (which falls back to solid). While this is mostly a Fontawesome Pro feature, it also affects free users as some regular icons are free to replace the previous '-o' icons, and then there are the brand icons.
To be able to selectively use a specific shape we would need several icon templates, not only a default one. E.g.:
Then we could enhance the magic icon syntax:
Well the first problem with this idea is that templates reside in a flat array so my nesting would not work. But just for the general idea.
What do you think about it and what design considerations would you have? I would be happy to prepare a pull request for this feature.