This module provides an API and render elements to use icons within Drupal 8. It defines a configuration entity type for icon sets and a plugin type for Icon Libraries.

But to make sure everybody won't start empty handed, a set of sub modules is included to integrate icons Icomoon and Fontello from the start.

When enabling the module, choose which submodule you want to use, or provide your own plugin for a custom icon library. For now icon sets from fontawesome for example is not supported, but it is pretty easy to do this based on the submodule fontello or icomoon.

After enabling the module(s) you can define an icon set from the interface. Just go to:
admin/appearance/icon_set

Choose "Add an icon set". Choose a name and the library plugin for icons. And fill in the settings needed for the
specific plugin. For instance, fontello and icomoon require you to specify a local path to the folder where the css,
json and fonts are located.

After doing this you can use icons from the interface for Menu Link Content and Views Menu Links.

Beware!!! Using multiple icon sets, especially those based on the same CSS could give conflicts in naming and styling. So when generating these sets make sure the class prefixes in css for instance are different between those sets if you use multiple sets on a single page.

To use an icon through a cusotm render array is pretty easy as well.
Just build a render array like this:

$icon = [
  '#type' => 'icon',
  '#icon_set' => 'your_icon_set_configuration_entity_id',
  '#icon_name' => 'name_of_the_icon_in_the_set',
];

// Or you could use a combination of the configuration entity id with the icon name like this:

$icon = [
  '#type' => 'icon',
  '#icon_id' => 'your_icon_set_configuration_entity_id:name_of_the_icon_in_the_set',
];

Icons Project page

git clone --branch 8.x-1.x https://git.drupal.org/sandbox/jbertoen/2850572.git icons

Manual review of other projects

  1. https://www.drupal.org/node/2849320#comment-11935714
  2. https://www.drupal.org/node/2850913#comment-11935750
  3. https://www.drupal.org/node/2850384#comment-11935784

Comments

jbertoen created an issue. See original summary.

PA robot’s picture

PA robot commented
Status: Needs review » Needs work

There are some errors reported by automated review tools, did you already check them? See http://pareview.sh/pareview/httpsgitdrupalorgsandboxjbertoen2850572git

We are currently quite busy with all the project applications and we prefer projects with a review bonus. Please help reviewing and put yourself on the high priority list, then we will take a look at your project right away :-)

Also, you should get your friends, colleagues or other community members involved to review this application. Let them go through the review checklist and post a comment that sets this issue to "needs work" (they found some problems with the project) or "reviewed & tested by the community" (they found no major flaws).

I'm a robot and this is an automated message from Project Applications Scraper.

jefuri’s picture

jefuri commented
Status: Needs work » Needs review
lendude’s picture

lendude
Primary language Dutch
Location Amsterdam
commented
Status: Needs review » Needs work

Looks really good after a quick scan.

review of the .module file:

function icons_help($route_name, RouteMatchInterface $route_match) {
Please update the help.

/**
 * Implements hook_form_FORM_ID_alter().
 */
function icons_form_views_ui_edit_display_form_alter(array &$form, FormStateInterface $form_state, $form_id)

Needs to be in the format Implements hook_form_FORM_ID_alter() for FORM_ID.
Also this function could use some explaining what is going on here. Also the reference in the readme could put some more emphasis on this feature. This would also benefit from a test.
In that function what does this unset($form['options']['menu']['context']['#suffix']); do?

/**
 * Implements hook_form_menu_link_content_form_alter().
 */

Needs to be in the format Implements hook_form_FORM_ID_alter() for FORM_ID.
Also the reference in the readme could put some more emphasis on this feature. This function could use more documentation. Could also benefit from a test.

$options = $display->getoption('menu');
getoption -> getOption

In icon_setschema.yml

    settings:
      type: icons.icon_set.settings.[%parent.plugin]

This looks really strange to me, that seems a bit circulair.

Will hopefully have some more time to look at the rest

lendude’s picture

lendude
Primary language Dutch
Location Amsterdam
commented

package: icons
The main module uses 'icons' and the submodules use 'Icons' so they are in different fieldsets in the module page.

Fatal error: Class 'Drupal\options\Plugin\Field\FieldType\ListStringItem' not found in /Applications/MAMP/htdocs/d8/drupal/modules/2850572/src/Plugin/Field/FieldType/ListIconItem.php on line 21
Missing dependency on the Options module

Notice: Undefined index: #icon_set in Drupal\icons\Element\Icon::preRenderIcon() (line 65 of modules/2850572/src/Element/Icon.php).
Notice thrown when configuring a new field without a working Icon set. Any way to prevent the use of the field when no sets have been configured? Currently it will just thrown notices and warnings and generally not work.

Next up, trying to get an icon set working (no luck so far, but that might not have anything to do with the module :)).

jefuri’s picture

jefuri commented
Status: Needs work » Needs review

Fixed all the mentioned comments by lendude. Reviewed it again through pareview.sh and processed those results as well.

Mirnaxvb’s picture

Mirnaxvb commented
Status: Needs review » Needs work

First of thanks for the clear description of the module how to implement it. This made it easy to do some quick functional tests. There are a few spelling mistakes in the readme though, for example:

“To use an icon through a cusotm render array is pretty easy as well.”

So i started with just a functional test with the Icomoon icon set and noticed a few things:

1: If you don’t make a selection after opening the form the selection automatically resets to the first option in the selection list, for example:

After selecting a prefix and suffix icon in for a menu link and saving the form. Edit the same link and instantly save the link again the Suffix and prefix both reset to the first option in the icon selection.

2: After selecting only a prefix saving the form and opening it again, the suffix all the sudden shows the first option of the selection list as selected.

3: After you made a selection of a prefix and a suffix you cant reset it back to none, for example:

After selecting a prefix and suffix icon in for a menu link and saving the form. Edit the same link and select “- none -“ for both prefix and suffix and save the link. Open the link again and it will show the previous selection.

I did not had time to look at all the code right now, hopefully i will have more time later on this evening.

Beside the comment from above a nice feature to have might be to enable/disable all icons on the Menu level, if you have time think it over :)

jefuri’s picture

jefuri commented
Status: Needs work » Needs review

Fixed the issues that mirnax experienced using the menu link content form.

lendude’s picture

lendude
Primary language Dutch
Location Amsterdam
commented
Status: Needs review » Needs work

It seems the icon set CSS file isn't always loaded on every page when needed, so icons don't always show even though the correct wrappers are added to a menu link.

michielnugter’s picture

michielnugter commented

JavaScript review:

1. Make more use of selector cache:
- $('.icon-picker > .item-list > ul', $formItem);
This selector is the base for various other selectors, cache it in a variable and use find() on it for other selectors.

- $(this)
This should also be stored in a variable.

2. Optimize css selectors

- Are the > in the css selectors really required?
- Use as little nesting as possible.
- Avoid tags in selectors.

Example:
$('.icon-picker > .item-list > ul li.selected', $formItem)

Would it be possible to make it like this? Not sure, haven't tried it.
$('.icon-picker .selected', $formItem)

3. Line 30: var icon_label = $('.icon-picker > .item-list > ul li.icon-item', $formItem).first().html();

Can be moved to an else statement on if ($selectedListItem.length > 0) {

4. Incorrect documentation

Copy paste?

  /**
   * Automatically refresh all channels every 3 seconds.
   */
jefuri’s picture

jefuri commented

Lendude's issue is fixed. Used renderPlain instead of render on the renderer service. Because of that the libraries would not attach.

michielnugter, could you redo the review on my javascript?

I cached most variables and pass those in the click event. Please check if my approach is acceptable.

I do not agree (or understand why) about your comment of the var label by the way. The way I did it only overrides label variable when the condition is met. If not the default label value is based on that situation. Maybe you could clarify the reason why?

var icon_label = $('.icon-picker > .item-list > ul li.icon-item', $formItem).first().html();
jefuri’s picture

jefuri commented
Status: Needs work » Needs review
lendude’s picture

lendude
Primary language Dutch
Location Amsterdam
commented
Status: Needs review » Reviewed & tested by the community

@jbertoen++ for adding the README's to the submodules, great addition.

All feedback has been addressed, I don't see any major issues anymore, so RTBC.

jefuri’s picture

jefuri commented
Issue summary: View changes
jefuri’s picture

jefuri commented
Issue summary: View changes
jefuri’s picture

jefuri commented
Issue summary: View changes
jefuri’s picture

jefuri commented
Issue tags: +PAreview: review bonus

Reviewed 3 other projects for the review bonus

michielnugter’s picture

michielnugter commented

JavaScript rework looks good, all issues have been fixed. I like the restructuring.

The comment you disagreed on is not relevant anymore due to the restructuring. Before you queried the dom, retreived a value and in some cases didn't use it. Always try to avoid the DOM if you can.

Good to see the PAReview bonus!

Mirnaxvb’s picture

Mirnaxvb commented
Status: Reviewed & tested by the community » Needs work

Manual Review

Individual user account
Yes: Follows the guidelines for individual user accounts.
No duplication
Yes: Does not cause module duplication and/or fragmentation.
Master Branch
Yes: Follows the guidelines for master branch.
Licensing
Yes: Follows the licensing requirements.
3rd party assets/code
Yes: Follows the guidelines for 3rd party assets/code.
README.txt/README.md
No: Does not follow the guidelines for in-project documentation and/or the README Template.
Code long/complex enough for review
Yes: Follows the guidelines for project length and complexity.
Secure code
Yes: Meets the security requirements.
Coding style & Drupal API usage
  1. Just a recommendation: A nice feature to have might be to enable/disable all icons on the Menu level
  2. Although there is no duplication for D8 there is a similar module in D7 https://www.drupal.org/project/icon it might be nice to add a "Similar projects and how they are different"

The starred items (*) are fairly big issues and warrant going back to Needs Work. Items marked with a plus sign (+) are important and should be addressed before a stable project release. The rest of the comments in the code walkthrough are recommendations.

If added, please don't remove the security tag, we keep that for statistics and to show examples of security problems.

This review uses the Project Application Review Template.

jefuri’s picture

jefuri commented
Status: Needs work » Needs review

Processed all review comments from Mirnaxvb. Recommendation about the menu configuration of enabling/disabling icons is added to the roadmap.

Mirnaxvb’s picture

Mirnaxvb commented
Status: Needs review » Reviewed & tested by the community

All feedback has been addressed, so far i can see no more major issue so RTBC.

jefuri’s picture

jefuri commented
Priority: Normal » Major
avpaderno’s picture

avpaderno
he/him
Primary language Italian
Location Brescia, 🇮🇹 🇪🇺
commented
Assigned: Unassigned » avpaderno
Priority: Major » Normal
Status: Reviewed & tested by the community » Fixed

Thank you for your contribution!

I am going to update your account so you can opt into security advisory coverage now.

These are some recommended readings to help with excellent maintainership:

You can find more contributors chatting on the IRC #drupal-contribute channel. So, come hang out and stay involved.

Thank you, also, for your patience with the review process.
Anyone is welcome to participate in the review process. Please consider reviewing other projects that are pending review. I encourage you to learn more about that process and join the group of reviewers.

I thank all the dedicated reviewer(s) as well.

avpaderno’s picture

avpaderno
he/him
Primary language Italian
Location Brescia, 🇮🇹 🇪🇺
commented

Status: Fixed » Closed (fixed)

Automatically closed - issue fixed for 2 weeks with no activity.