No icons shown

Work in progress patch. Now adding the links from a template.

I have ported the full "icon experience" from the Drupal 7 module. The text links are now icons. Clicking on those icons triggers an Ajax call to register the vote and refreshes the vote count. I did a bit more too and made it so the status/warning messages are also added via the Ajax call, which wasn't done in the Drupal 7 module. It's fully functional, but there still seems to be some caching issues that I can't figure out. If voting randomly multiple times, I've noticed that once in a while a vote doesn't get properly registered or canceled. It seems to appear randomly.

We also need to discuss how the icons should be displayed. Currently, we have red thumb down and up icons and gray thumb down and up icons. When voting, a css class is dynamically added to switch to the gray icons. However, when refreshing the page, the red icons are still displayed. Furthermore, right now, the gray icons are not clickable. This is not consistent with the fact that users can cancel their votes. Screenshot (2 is after Ajax call, 3 is after a page refresh) :

(sorry for the lack of Gimp skills)

I think what would make more sense would be to have a green thumb up icon, a red thumb down icon and somehow "highlighted" green thumb up and red thumb down icons. After voting, the "highlighted" icon is used for the choice that was chosen. The user can still click on either type of icon to cancel his vote. Any other suggestion?

I'm not a visual person, so I have no strong preference there.

I expect many sites will want to customize this anyway. So what's more important than the look is that will work well.

I'm also fine with a follow-up issue to change the icon color and behavior. in general, I like your idea. For highlighting, we could use filled and non-filled tokens. I think it should also only highlight the icon you actually clicked, to indicate your choice, not both. But again, lets get the functional part right first.

1. +++ b/like_and_dislike.js
   @@ -0,0 +1,26 @@
   +}).call(this);
   

   haven't seen that kind of wrapper before, no idea what it does :)

2. +++ b/like_and_dislike.module
   @@ -51,43 +51,21 @@ function like_and_dislike_entity_view(array &$build, EntityInterface $entity, En
   +      '#like_status' => $account->hasPermission("add or remove like votes on $entity_type_id"),
   +      '#dislike_status' => $account->hasPermission("add or remove dislike votes on $entity_type_id"),
   

   that seems misleading, that's not the status but wheter you can like or not. I'd name those _permission.

   _status would then imho be whether the user did vote this, which should also solve the problem of not having the class after a refresh. could also be a single one that contains nothing, like or dislike as a string.

3. +++ b/like_and_dislike.module
   index 0000000..83e3fac
   --- /dev/null
   
   --- /dev/null
   +++ b/like_and_dislike_service.js
   
   +++ b/like_and_dislike_service.js
   +++ b/like_and_dislike_service.js
   @@ -0,0 +1,36 @@
   
   @@ -0,0 +1,36 @@
   +(function() {
   

   can we put the js files into a separate js folder?

4. +++ b/like_and_dislike_service.js
   @@ -0,0 +1,36 @@
   +      return jQuery.ajax({
   

   usually jQuery/Drupal/drupalSettings are passed in and there is only a single wrapper. check core JS files in modules.

   also, I think it is more common to register functions like this on Drupal and not on globally. e.g. Drupal.ajax.

   we should ask someone who is more familiar with JS to review this.

5. +++ b/src/Controller/VoteController.php
   @@ -42,12 +53,22 @@ class VoteController extends ControllerBase implements ContainerInjectionInterfa
   +      return new Response($entity->like . '/' . $entity->dislike . '/status/Your ' . $vote_type_text . ' vote was added.');
   

   this is not translatable like this, also, usually json is used to return data instead of custom encoded strings. should be easier to parse. (see JsonResponse)

6. +++ b/templates/like-and-dislike-icons.html.twig
   @@ -0,0 +1,44 @@
   +  <span style="display:none" class="throbber">Loading...</span>
   

   use |t to translate this.

I made most of these changes.

I think it should also only highlight the icon you actually clicked [...].

Yes, that's what I meant. I'll open a new issue to discuss and work on this.

that seems misleading, that's not the status but wheter you can like or not. I'd name those _permission.
_status would then imho be whether the user did vote this, which should also solve the problem of not having the class after a refresh [...].

I already did that, but we don't want the icons permanently disabled after voting, cause users can still cancel or change their vote. Maybe we can leave this like this for now (icons disabled after voting, enabled again after a refresh) and do this properly in the other issue?

use |t to translate this.

Everything should be properly translatable now.

Also, the click() event was always called twice. I'm not exactly sure why. The Javascript is attached in like_and_dislike_entity_view(), which will likely be called multiple times. But, even in cases this function is called only once, the click() event was triggered twice. I fixed that by unbinding the click function from the anchor tag before binding it. This caused the glitchiness I observed earlier.

For highlighting, we could use filled and non-filled tokens.

Tokens?

Changed this so that the icons are still clickable after voting and remain red. If we still want them clickable, I think it would make more sense not to turn them gray. So voting doesn't visually affect the icons now. The icons are only gray and unclickable if the user doesn't have permission to vote. Still need to discuss in #2773341: Proper look and behaviour for like and dislike icons. if we want the selected vote somehow "highlighted" / better visual feedback after voting.

1. +++ b/src/Controller/VoteController.php
   @@ -68,7 +68,12 @@ class VoteController extends ControllerBase implements ContainerInjectionInterfa
   +        'message' => t('Your @vote_type vote was added.', ['@vote_type' => $vote_type_text]),
   

   as mentioned in the unlike issue, this is not valid, the full sentence needs to be translatable as a while, or it will not be possible to do it correctly in some languages. And as written there, not sure how much you already did there, it could be simplified to avoid "vote". that we use a voting api internally is an implementation detail.

2. +++ b/templates/like-and-dislike-icons.html.twig
   @@ -22,23 +22,23 @@
   -  <span style="display:none" class="throbber">Loading...</span>
   +  <span style="display:none" class="throbber">Loading...|t</span>
    </div>
   

   I think that's just printing it out as a string ;)

   You need something like {{ 'Book traversal links for'|t }}

[...] it could be simplified to avoid "vote".

Actually, what about just "Your vote was added." / "Your vote was canceled."?

I think that's just printing it out as a string ;)

Ha ! Never noticed it !

The binary files are missing in the patch.

.like-and-dislike-container a {
width: 21px;
height: 21px;
text-indent: -9999px;
background: url("../images/icons.png") 0 0 no-repeat;
cursor: pointer;
}

Missing icons.png

Added images.

Huh, test fixes...

Patch doesn't apply anymore but should be easy to resolve.

Also quite hard to review, so I'll just add some general notes.

* Quickly tests, basically works. lets get this in asap, but we'll need to work on #2773341: Proper look and behaviour for like and dislike icons. then for an acceptable user experience.
* I don't like the like_permission keys, lets make this like_access and dislike_access.
* the access check logic doesn't seem very reliable, it just depends on a class being there or not. maybe we could leave the JS/library out completely if the user doesn't have access, also a performance improvement. The controller should also again check access.
* resetCache()/reload of the entity seems overkill. just increment/decrement the property based on the action that happened. view builder reset is necessary.
* not all messages in JsonResponse are translatable.

Unrelated, for separate follow-up issues (possible student novice issues):

* No idea what LikeDislikeAccessController is for, doesn't seem to be used.
* multiple uses of entity (type) manager in controller, should be injected

#15 should be addressed with this patch. Sorry for long patch...

Will create followups soon.

Thanks, I think this is a good first step, lets continue in the follow-up to improve the UI.

Created follow-ups: #2800679: Inject entity managers/services through the module and #2800677: Remove like and dislike access controller