Problem/Motivation

Main info from parent issue:

Use of theme functions is discouraged in D8 and removed in D9 in favor of using Twig templates.

All theme function in includes/fivestar.theme.inc should be replaced by templates in templates/ and hook_theme() should be modified to use these templates.

Additional info:

Current output looks next:

<div class="fivestar-">
    <div class="fivestar-widget-static fivestar-widget-static- fivestar-widget-static-5 clearfix">
        <div class="star star-1 star-odd star-first"><span class="on">3</span></div>
        <div class="star star-2 star-even"><span class="on"></span></div>
        <div class="star star-3 star-odd"><span class="on"></span></div>
        <div class="star star-4 star-even"><span class="off"></span></div>
        <div class="star star-5 star-odd star-last"><span class="off"></span></div>
    </div>
</div>

The problem exists in the next lines:

<div class="fivestar-">
    <div class="fivestar-widget-static fivestar-widget-static- fivestar-widget-static-5 clearfix">

And even more closely:

fivestar-
fivestar-widget-static-

Above we can see that in class names we not display all the needed information. In the current case, we do not display information about voting and widget types. At the moment we don't get the information in the 'fivestar_static' theme function.

Proposed resolution

Move static theme output into twig template and fix related problems.

CommentFileSizeAuthor
#7 interdiff_4-7.txt3.62 KBinit90
#7 3142319-7.patch8.25 KBinit90
#4 interdiff_2-4.txt2.25 KBinit90
#4 move_static_theme_output_into_twig-3142319-4.patch7.98 KBinit90
#2 move_static_theme_output_into_twig-3142319-2.patch6.06 KBinit90

Comments

init90 created an issue. See original summary.

init90’s picture

init90
Location Ukraine
commented
Title: Incorrect output of static rating » Move static theme output into twig templete
Issue summary: View changes
Parent issue: » #3133125: [Meta] Replace theme functions with Twig templates
StatusFileSize
new move_static_theme_output_into_twig-3142319-2.patch6.06 KB

Here migration of static output theme function into a twig. Still exist the problem which described in issue summary.

init90’s picture

init90
Location Ukraine
commented

Closed #3142322: Attach necessary library for static rating output in order to make changes from it in the current issue.

init90’s picture

init90
Location Ukraine
commented
StatusFileSize
new move_static_theme_output_into_twig-3142319-4.patch7.98 KB
new interdiff_2-4.txt2.25 KB

Fixed incorrect theme output.

tr’s picture

tr commented

This is blocked by #3136366: hook_fivestar_widgets() implementations should explicitly include their associated libraries because until that goes in we don't have a library to #attach.

Also, this still needs to add the widget name variable to the preprocess so it's available in the template, right?

Also, I don't think we should be doing calculations in templates - that's fragile, because when someone overrides the template they might get the calculation wrong, and it mixes the computation of the data with the presentation of the data. My rule is to do all the calculation in preprocess functions and make the results of those calculations available as variables which may be used in the template. The template should only worry about adding the correct markup around the variables - if I want to change the markup I shouldn't have to understand how Fivestar calculates numbers. In this case, that means moving the calculation of star_value, prev_star_value, and percent into the preprocess function.

init90’s picture

init90
Location Ukraine
commented

@TR thanks for your feedback.

Also, this still needs to add the widget name variable to the preprocess so it's available in the template, right?

yes, patch #4 contain necessary changes.

Also, I don't think we should be doing calculations in templates - that's fragile, because when someone overrides the template they might get the calculation wrong, and it mixes the computation of the data with the presentation of the data. My rule is to do all the calculation in preprocess functions and make the results of those calculations available as variables which may be used in the template. The template should only worry about adding the correct markup around the variables - if I want to change the markup I shouldn't have to understand how Fivestar calculates numbers. In this case, that means moving the calculation of star_value, prev_star_value, and percent into the preprocess function.

Good point, totally makes sense. Tomorrow I'll work on it.

init90’s picture

init90
Location Ukraine
commented
Status: Active » Needs review
StatusFileSize
new 3142319-7.patch8.25 KB
new interdiff_4-7.txt3.62 KB

Added changes according to #5

Also, I think that this issue and #3136366: hook_fivestar_widgets() implementations should explicitly include their associated libraries can go in parallel. Here we don't change behavior only fix some class naming.
And #3136366: hook_fivestar_widgets() implementations should explicitly include their associated libraries has the next code in src/Element/Fivestar.php

    if ($widget_name != 'default') {
      $element['#attached']['library'][] = \Drupal::service('fivestar.widget_manager')->getWidgetLibrary($widget_name);
    }

which should fix the problem with the library attach.

  • TR committed 4ae9d70 on 8.x-1.x authored by init90 
    Issue #3142319 by init90:  Move static theme output into twig templete
tr’s picture

tr commented
Category: Bug report » Task
Status: Needs review » Fixed

Thanks. Committed.

Status: Fixed » Closed (fixed)

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