Skip to content

Comments

[4.0] Improvement Vote Plugin#31098

Merged
rdeutz merged 96 commits intojoomla:4.0-devfrom
sandewt:patch-2
Apr 14, 2021
Merged

[4.0] Improvement Vote Plugin#31098
rdeutz merged 96 commits intojoomla:4.0-devfrom
sandewt:patch-2

Conversation

@sandewt
Copy link
Contributor

@sandewt sandewt commented Oct 15, 2020

Pull Request for Issue #30193.

Summary of Changes

In the current situation only two stars can be seen.
In the new situation, a half star has been added.
Font Awesome icons are used and no longer two (bad) .png images.

Testing Instructions

Install Multilingual Sample Data
Use npm ci (npm run build:css) or Prebuilt package (bottom page)

I.

  • Enable Vote Plugin
  • Article Edit > Options > Show Voting
  • Also create a menu link (list all categories) to an article.

Cast votes.

II.
Change the values in the table #__content_rating.
Values for testing: round = 0.5 (old round to 1)
rating 3.2 -> 3 stars
rating 3.3 -> 3.5 stars (10 : 3 = rating_sum / rating_count)
rating 3.4 -> 3.5 stars
rating 3.7 -> 3.5 stars
rating 3.8 -> 4 stars

III.
Test in a LFT and a RTL language.

IV.
Test overrides (images).

V.
Code review + security.

The role and aria-label attribute have been added for accessibility. See the html source code too.

Actual result BEFORE applying this Pull Request

Two stars can be seen: a whole and an empty star.
E.g. vote average = 3.3 becomes 3 stars.

Expected result AFTER applying this Pull Request

Three different stars can be seen: a whole, a half and an empty star.
E.g. vote average = 3.3 becomes 3.5 stars.

issue 31098

Documentation Changes Required

?

Additional comment

When PR #31096 is added, there are more options in terms of the look and feel of the stars.

[EDIT]

3 instead of 2 stars
fa-icons instead of .png images
@joomla-cms-bot joomla-cms-bot added the Language Change This is for Translators label Oct 15, 2020
@adj9
Copy link

adj9 commented Oct 15, 2020

The evaluation with whole function values. I did not understand how to modify the table for decimal ratings, should I make an ALTER TABLE? If yes, with what new attribute?

@sakiss
Copy link
Contributor

sakiss commented Oct 15, 2020

@sandewt seems like we have to alter the table's rating_sum to accept decimals. It only gets integers atm.

@jiweigert
Copy link

What about not altering the column (preferred) but using a conversion of 1-100 for 0.1-10.0 rating?
Possible?


This comment was created with the J!Tracker Application at issues.joomla.org/tracker/joomla-cms/31098.

@sandewt
Copy link
Contributor Author

sandewt commented Oct 15, 2020

@adj9 and @sakiss

I'm trying to explain

file: \components\com_content\src\Model\ArticleModel.php

Line 152: here round is 0 decimal, becomes 1 decimal

'ROUND(' . $db->quoteName('v.rating_sum') . ' / ' . $db->quoteName('v.rating_count') . ', 0) AS '
. $db->quoteName('rating'),	$db->quoteName('v.rating_count', 'rating_count'),

rating = rating_sum / rating_count (= average)

Suppose: rating_sum = 100 and rating_count = 30

  • round = 0 decimal
    100 : 30 = 3

  • round = 1 decimal (PR)
    100 : 30 = 3.3

  • round = 2 decimals
    100 : 30 = 3.33

issue 31098-2issue 31098-3

The final rating then becomes: $rating = round($rating / 0.5) * 0.5; (= on half stars)

variable $text is canceled
extra space
@sandewt
Copy link
Contributor Author

sandewt commented Oct 15, 2020

What about not altering the column (preferred) but using a conversion of 1-100 for 0.1-10.0 rating?
Possible?

There are many ways to indicate a rating. This PR is in line with the current situation. Yes, a lot is possible. But an apparently small change often gives a lot of work. You can always open a new issue with a new proposal.

style="margin-left: -1.2em becomes style="margin-left: -1.15em
@sandewt sandewt changed the title [4.0] Improvent Vote Plugin [4.0] Improvement Vote Plugin Oct 16, 2020
Add semicolon
@sandewt
Copy link
Contributor Author

sandewt commented Oct 16, 2020

Some checks were not successful

I can't find the cause. Need some help.

@infograf768
Copy link
Member

Restarted drone

@infograf768
Copy link
Member

infograf768 commented Oct 16, 2020

Please explain how to test if (($stars - floor($stars)) >= 0.5) to get a half star

@sandewt
Copy link
Contributor Author

sandewt commented Oct 16, 2020

Please explain how to test if (($stars - floor($stars)) >= 0.5) to get a half star

  • Go to the database
  • open the table #__content_rating
  • change eg the rating_sum in 100
  • change eg the rating_count in 30

rating = rating_sum : rating_count = 100 : 30 = 3.3, that results in 3.5 stars

issue 31098-4

@infograf768
Copy link
Member

It does not work in RTL because of

style="margin-left: -1.15em;"

LTR
Screen Shot 2020-10-16 at 11 20 41

RTL
Screen Shot 2020-10-16 at 11 29 27

  1. First, it's better to create a class instead of an inline style
  2. It will not be sufficient for RTL to use margin-right: -1.15em; as the half star will be placed in the wrong side

Screen Shot 2020-10-16 at 11 25 32

@infograf768
Copy link
Member

infograf768 commented Oct 16, 2020

To get a correct placement in rtl you need to reverse the color

if (Factory::getLanguage()->isRtl())
{											{
	$img .= '<span class="text-warning fas fa-star" aria-hidden="true"></span>';
	$img .= '<span style="margin-right: -1.15em;" class="text-muted fas fa-star-half" role="img" aria-label="' . Text::_('PLG_VOTE_STAR_ACTIVE_HALF') . '" aria-hidden="true"></span>';
}
else
{
etc.

and anyway a class to replace the inline style

to get

Screen Shot 2020-10-16 at 11 43 05

@sandewt
Copy link
Contributor Author

sandewt commented Oct 16, 2020

Thanks @infograf768 ,

and anyway a class to replace the inline style

You mean in: build/media_source/plg_content_vote/css ?

@infograf768
Copy link
Member

You mean in: build/media_source/plg_content_vote/css ?

I guess so and evidently loading it from media/plg_content_vote/css/ in rating.php
rating.css can be a good name

/** @var Joomla\CMS\WebAsset\WebAssetManager $wa */
$wa = $app->getDocument()->getWebAssetManager();
$wa->registerAndUseStyle('plg_content_vote', 'plg_content_vote/rating.css');
  • take off unused use Joomla\CMS\HTML\HTMLHelper; in the same file.

@N6REJ
Copy link
Contributor

N6REJ commented Nov 16, 2020

@sandewt no, one I made, but as @infograf768 no need to mess with copyright yourself. Simply do file_get_contents ( assuming your not using my helper ) on the svg file itself and your done. Auto updated and everything.

@sandewt
Copy link
Contributor Author

sandewt commented Nov 19, 2020

@sandewt no, one I made, but as @infograf768 no need to mess with copyright yourself.

I totally agree.

Simply do file_get_contents ( assuming your not using my helper ) on the svg file itself and your done. Auto updated and everything.

I wait and see if your PR [#31410] - [4.0] htmlhelper svg is successful.

back to an older situation
@sandewt
Copy link
Contributor Author

sandewt commented Nov 30, 2020

Back to an older situation:

See [4.0] Webauth inline SVG button: #31516 (comment)

See: [4.0] htmlhelper svg #31410

@infograf768
Copy link
Member

I have tested this item ✅ successfully on c0f9d3d


This comment was created with the J!Tracker Application at issues.joomla.org/tracker/joomla-cms/31098.

The check is now in the right place.
@sandewt sandewt mentioned this pull request Dec 2, 2020
@paritshivani
Copy link

I have tested this item ✅ successfully on c0f9d3d

Tested successfully


This comment was created with the J!Tracker Application at issues.joomla.org/tracker/joomla-cms/31098.

@sandewt
Copy link
Contributor Author

sandewt commented Dec 5, 2020

I have tested this item ✅ successfully on c0f9d3d

I've noticed there is a reference (c0f9d3d) to @sandewt sandewt committed on 19 Oct.
After that there have been changes in the code

How is that possible?

@rjharishabh
Copy link
Contributor

rjharishabh commented Mar 29, 2021

When I have tested this item, an error occurred

Chrome on Windows 10 with Xampp Server

@sandewt
Copy link
Contributor Author

sandewt commented Mar 29, 2021

@rjharishabh

I have tested this item 🔴 unsuccessfully on 8e697cd

What are the specifications of your test environment?

Please, check the error log.

[EDIT]

@infograf768
Copy link
Member

rtc after checking again.


This comment was created with the J!Tracker Application at issues.joomla.org/tracker/joomla-cms/31098.

@joomla-cms-bot joomla-cms-bot added the RTC This Pull Request is Ready To Commit label Mar 30, 2021
@infograf768 infograf768 added this to the Joomla 4.0 milestone Mar 30, 2021
controle file_exists
@sandewt
Copy link
Contributor Author

sandewt commented Apr 6, 2021

@rjharishabh please test

@rjharishabh
Copy link
Contributor

rjharishabh commented Apr 6, 2021

@sandewt I am getting this and the image is also not present
I don't know, am I doing something wrong

in both chrome and firefox

Screenshot_2021-04-06 Your Template

@sandewt
Copy link
Contributor Author

sandewt commented Apr 6, 2021

@sandewt I am getting this and the image is also not present
I don't know, am I doing something wrong

Looks like you're not doing anything wrong, check the following:

The images are .svg files.
It concerns these images (stars).

vote

The following classes are loaded, see the source code.
vote2

@rdeutz rdeutz merged commit c4e0f81 into joomla:4.0-dev Apr 14, 2021
@joomla-cms-bot joomla-cms-bot removed the RTC This Pull Request is Ready To Commit label Apr 14, 2021
@sandewt
Copy link
Contributor Author

sandewt commented Apr 14, 2021

Thanks all 👍

@sandewt
Copy link
Contributor Author

sandewt commented Apr 14, 2021

Related to:

#32919 | [4.0] css changes in voting plugin
#32914 | [4.0] Pagination set to bottom after voting

@rjharishabh
Copy link
Contributor

@sandewt 👍
thanks

mstrap pushed a commit to mstrap/joomla-cms that referenced this pull request Nov 16, 2021
@sandewt sandewt deleted the patch-2 branch January 12, 2023 12:00
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Language Change This is for Translators NPM Resource Changed This Pull Request can't be tested by Patchtester

Projects

None yet

Development

Successfully merging this pull request may close these issues.