User tests: Successful: Unsuccessful:
Removes redundant calculation of limit value in pagination.
Nothing
Status | New | ⇒ | Pending |
Category | ⇒ | Layout |
Labels |
Added:
?
|
What makes you feel it's not needed
If you look at the previous line, the content is being stored in a variable $limit.
if ($item->base > 0)
{
$limit = 'limitstart.value=' . $item->base;
}
else
{
$limit = 'limitstart.value=0';
}
Hence, it is meaningless to recalculate.
@brianteeman It seems the same calculation is already done here:
joomla-cms/layouts/joomla/pagination/link.php
Lines 57 to 64 in 1334f2b
So the PR could be right.
@rajkumaar23 Could you provide some testing instructions?
@brianteeman It seems the same calculation is already done here:
joomla-cms/layouts/joomla/pagination/link.php
Lines 57 to 64 in 1334f2b
.So the PR could be right.
@rajkumaar23 Could you provide some testing instructions?
Sorry, I'm new to Joomla. I have no idea about how tests work here. Could you please help ?
@rajkumaar23 Just add some testing instructions for human testers to the description in your PR where the head lines from the PR template say so:
For example you could simply write "Code review" below "Testing inctructions".
If you want to be sure that you made no mistake which could be missed by some reviewer you also could write instructions for a real test by a human, e.g.:
(or so)
Here for this PR it is trivial, but our contribution rules require to fill out the necessary information in the description.
Thank you for your PR.
@richard67
Sure, thanks. I've updated my PR description
Title |
|
@rajkumaar23 When we make PRs for the 4.0-dev branch we're used to have "[4.0] " at the beginning of the title, so we can quickly see it is not for staging (= 3.9.x-dev) or 3.10-dev. Is not a must but good practice. I've just updated the title for you. Will test soon, but as I could already see by code review I am sure the result will be success.
@richard67 Sure, I'll follow that format. And, thanks for the help :)
@rajkumaar23 Another suggestion: I see you were using your 4.0-dev branch for this PR. This is ok as long as you do not make several PRs for the 4.0-dev branch of Joomla.
But as you maybe can imagine, new contributors are always welcome, and so if you have some free time and find other issues to fix you might make more than one PR at a time. We (Joomla community) would be happy about that.
For working on several issues it will be necessary to create a new branch for making a PR. Depending on the branch for which the PR will be, you will make this new branch based on the 4.0-dev branch or the staging branch of the CMS.
Maybe you know all that already and this time it was just faster not to do it. But if that is new to you, you can find some useful information here: https://docs.joomla.org/Using_the_Github_UI_to_Make_Pull_Requests and here https://docs.joomla.org/Git_branching_quickstart and at a few other places linked on https://docs.joomla.org/Portal:Joomla!_Code_Contributors.
I have tested this item
I have tested this item
Status | Pending | ⇒ | Ready to Commit |
RTC
Labels |
Added:
?
|
Thanks for your first PR to Joomla
Status | Ready to Commit | ⇒ | Fixed in Code Base |
Closed_Date | 0000-00-00 00:00:00 | ⇒ | 2020-01-11 17:11:49 |
Closed_By | ⇒ | wilsonge |
Thanks for your first PR to Joomla
?
Thank you! ?️ Looking forward to contribute more.
What makes you feel it's not needed