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

Add possibility to use FontAwesome icons instead of Glyphicons #152

Closed
wants to merge 2 commits into from
Closed

Conversation

alphp
Copy link
Contributor

@alphp alphp commented Apr 29, 2018

No description provided.

@codecov
Copy link

codecov bot commented Apr 29, 2018

Codecov Report

Merging #152 into master will increase coverage by 0.1%.
The diff coverage is 100%.

Impacted file tree graph

@@             Coverage Diff             @@
##             master     #152     +/-   ##
===========================================
+ Coverage     88.91%   89.02%   +0.1%     
- Complexity      368      372      +4     
===========================================
  Files            19       19             
  Lines          1146     1157     +11     
===========================================
+ Hits           1019     1030     +11     
  Misses          127      127
Impacted Files Coverage Δ Complexity Δ
src/View/Helper/HtmlHelper.php 85.53% <100%> (+1.07%) 43 <1> (+4) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update ef50df4...8e5de7f. Read the comment docs.

@Holt59
Copy link
Collaborator

Holt59 commented Apr 29, 2018

Thanks for the PR.

I've intentionally removed font-specific method in a previous version because I think it is up to the user of the helpers to add their own templates.

We are currently working on a dedicated IconHelper to ease the process of customizing the fonts, see issue #149.

For these reason I have to decline the PR in its actual state, feel free to comment on the issue if you have idea regarding the implementation.

@Holt59 Holt59 closed this Apr 29, 2018
@alphp
Copy link
Contributor Author

alphp commented Apr 29, 2018

I'm not sure that a new helper is needed for the icons.
However, I think it is better to change the current HTML helper template for the icons to:
'icon' => '<{{tag}} aria-hidden="true" class="{{font}}{{type}}{{attrs.class}}"{{attrs}}></{{tag}}>',
And the icon function could look like this:


  public function icon($icon, array $options = []) {
       $tag = empty($options['tag']) ? 'i' : $options['tag'];
       $fonts['glyphicon'] = 'glyphicon glyphicon-';
       $fonts['fa'] = 'fa fa-';
       $fonts['fas'] = 'fas fa-';
       $fonts['fab'] = 'fab fa-';
       $font = 'glyphicon glyphicon-';
       if (!empty($options['font']) and isset($fonts[$options['font']])) {
           $font = $fonts[$options['font']];
       }
       unset($options['tag']);
       unset($options['font']);

       $options += [
           'templateVars' => []
       ];
       return $this->formatTemplate('icon', [
           'tag' => $tag,
           'font' => $font,
           'type' => $icon,
           'attrs' => $this->templater()->formatAttributes($options),
           'templateVars' => $options['templateVars']
       ]);
   }

The array $fonts could be a property of the extendable class at runtime, such as templates. Although I have no idea how this should be implemented.
In this way FontAwesome and Glyphicon would be supported as other sources without modifying the code.

I hope my ideas serve to improve your helpers as much as they have helped me.

@alphp
Copy link
Contributor Author

alphp commented Apr 29, 2018

If the idea is to create a new helper for the icons I think FontAwesome support should not be removed from the HTML helper until the new ICON helper replaces it.

When the support was removed without an alternative, I caused a small inconvenience, since in my projects I usually use both FontAwesome and Glyphicon. The solution to change the template does not seem appropriate when you switch from one source to another repeatedly, it is not clear and sometimes you do not know what source you are using.

At least I think so.

@Holt59
Copy link
Collaborator

Holt59 commented Apr 30, 2018

@alphp The goal of the Icon helper is to not rely on the Html helper for the easy-icon feature. We are going for something more generic and more customizable.

The helper will manage a set of templates (possibly with a non-linear organization) so that you would be able to select one easily when needed and to use them with the easy-icon feature.

We will also likely add a special __call or even some runtime created method so that, e.g., faIcon or glIcon would automatically be generated and switch to the appropriate template. This is yet to be decided.

In the mean-time, you can create your own HtmlHelper inheriting the Bootstrap.HtmlHelper to add the features you want (or even faIcon or glIcon methods).

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

Successfully merging this pull request may close these issues.

2 participants