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

module articles category - add random order option #8538

Closed
wants to merge 3 commits into from
Closed

module articles category - add random order option #8538

wants to merge 3 commits into from

Conversation

alikon
Copy link
Contributor

@alikon alikon commented Nov 25, 2015

follow up #6648
add random order option to articles category module

How to test

apply the patch
select the random option on Article Field to Order By
modules-articles-category

Expected result

the articles are showed randomly orderdered

Additional comment

should work on all supported db's

$articles->setState('list.ordering', $params->get('article_ordering', 'a.ordering'));
$articles->setState('list.direction', $params->get('article_ordering_direction', 'ASC'));
$ordering = $params->get('article_ordering', 'a.ordering');
if (trim($ordering) == 'rand()')
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

can we add a clean line bevor the if? I think as we don't require rand() we should use random as option value as i think it is better readable what do you think?

@zero-24
Copy link
Contributor

zero-24 commented Nov 26, 2015

I have just comment on two issues. I can test it later today ;)

@zero-24
Copy link
Contributor

zero-24 commented Nov 26, 2015

I have tested this item ✅ successfully on eb2ddf1

It works. Lets work out the two inline comments ;)


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

Nicola Galgano added 2 commits November 27, 2015 17:49
as for comment use random instead of rand()
as per comment add a blank line before if and changed rand() to random
@alikon
Copy link
Contributor Author

alikon commented Nov 27, 2015

comments should be ok now ;)

@zero-24
Copy link
Contributor

zero-24 commented Nov 27, 2015

Great 👍

@anibalsanchez
Copy link
Contributor

I have tested this item ✅ successfully on 72d60ce

Test OK


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

@brianteeman
Copy link
Contributor

Thanks for testing - setting RTC


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

@joomla-cms-bot joomla-cms-bot added the RTC This Pull Request is Ready To Commit label Dec 10, 2015
@pjwiseman
Copy link
Contributor

I have tested this item ✅ successfully on 72d60ce

Piggy-backed of zero's test with a code review of some minor changes. Looks good.


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

@rdeutz rdeutz added this to the Joomla 3.5.1 milestone Jan 6, 2016
@wilsonge wilsonge modified the milestones: Joomla! 3.5.0, Joomla 3.5.1 Jan 16, 2016
@wilsonge wilsonge closed this in daa6035 Jan 16, 2016
@alikon alikon deleted the another-rand branch January 17, 2016 06:39
@wilsonge wilsonge removed the RTC This Pull Request is Ready To Commit label Jan 17, 2016
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
Projects
None yet
Development

Successfully merging this pull request may close these issues.

9 participants