User tests: Successful: Unsuccessful:
Pull Request for Issue #21676.
The "default value" for list limit moved out of the list-container. The default value comes from the Javascript and not from the filter-form. I'm not quite happy with this solution because it makes it impossible to exclude the list values from being cleared. (sorting and number of items). To achieve that a change of HTML-code and CSS-styling would be neccessary . I know now why CSS-styling shouldn't use the Javascript selectors.
Before installing this fix:
Rerun the list and click clear.
The list limit should now be 5. (be aware - list could have bin cached)
Extra check - set the value in the view-tmpl
add: $searchtoolsOptions = array('defaultLimit' => 50);
edit to :
echo LayoutHelper::render('joomla.searchtools.default', array('view' => $this,'options' => $searchtoolsOptions));
Rerun and the value after Clear should be 50.
Shows all items.
No
Status | New | ⇒ | Pending |
Category | ⇒ | JavaScript Repository NPM Change |
Labels |
Added:
NPM Resource Changed
?
|
@richard67 In what view did you test? I'll test that myself.
I have tested this item
@richard67 Thanks!
Title |
|
@dgrammatiko
Do you know if the code for the list-container Is needed for alternative layouts? It's not used in the core layout.
As I wrote: This fix always clears the No Items to the "default" value if it's present.
@richard67 and all
Ready for test. jQuery was removed after your test. That should not change anything.
I have tested this item
Do you want me to add a fix for the Number of Items to this PR?
The priority would be 1. from the view, 2. from the filter-form 3. from global parameter, 4. fixed 20
As is the 2. is missing. I was wrong about this value coming fixed from the Javascript where 20 is predefined.
I'm picking around a bit trying to find a better way to handle the merged list- and filter-values. That would be a new PR if I'm able to come up with an idea.
@richard67
It was a general question if it's preferred to include a second issue or not.
To better understand what I mean, you can edit a filter form and set the limit to e.g. 5.
Better with the first fix installed.
e.g. in administrator/components/com_plugins/forms/filter_plugins.xml
Change line 78 to default="5"
Go to Plugin List. Check the list limit (=5)
Click on Clear button.
I expect the list limit to be 5.
What is it.
(personally I would prefer a No Change but that's another issue.)
@schnuti And now with this PR included it is 20? I don't remember the test right now.
I am not sure what is better: Make a new PR or do it in this PR here.
If you can fix it soon I would say do it in this one here, but if it takes longer I would say make new issue, so this PR here gets tested and merged soon.
I have a working solution for the list limit value but have to do some tests on fictive cases.
The code looks a bit weird because of those other cases.
Let us know when ready so we don't test while still in progress.
@schnuti Let us know if you wanna fix that in this PR so a 2nd tester waits with testing or if you make a new one so we quickly get a 2nd tester for this one for RTC. By the way I am happy to see you contributing to Joomla and would of course be happy if it would be more
@richard67
You'll have it in a few hours. The extra tests worked fine for me.
Category | JavaScript Repository NPM Change | ⇒ | JavaScript Repository NPM Change Layout |
Ready for test if the checks go through.
Do you need some test instructions?
Do you need some test instructions?
Would be helpful, yes.
Test instructions added on the top.
I will test tomorrow, today am busy for the rest of the day.
@schnuti Test with changed default value in XML works only when default value in XML is a multiple of 5, e.g. with 5 or 10 or 15 ... => OK, but when e.g. 13: List shows all plugins, list limit in filter field shows 5 => Strange, but maybe desired behavior because limits shall be multiple of 5.
But then it should be fixed by either validating the value, or integer division by 5 in code or round in code, or at least leave a comment in the xml. I am for integer division, i.e. e.g. 13 results in 10.
Can you fix that?
@richard67
Is it when you click Clear or when you first start the list?
The changed scripts do not manipulate the values but perhaps the field type.
I'll have a look tomorrow.
Without my PR part 2 the XML value is not used as default in the Javascript.
Yes, I know. I mean when you change somewhere else the value to e.g. 13 you will have the problems without your PR, too.
Maybee try to set the Global list limit to 13 without PR II.
I will try tomorrow.
@richard67
I added a check for allowed values. It's not bullet prof if someone somehow sets custom values.
The problem is probably that when the JS writes to the "Choices" list, the value isn't there.
Maybe the check could be done against the DOM within the JS instead? I do not know.exactly how to do that. Is it worth it?
I also don't know exactly how to do that. No idea if it is necessary or worth it. Will test tomorrow. Good night.
@schnuti The more I think it over, the more I am not sure if your last change is really needed. I think old code also relies on the default value in the XML being set in the right way. I suggest just undo that last commit, update your testing instructions to use values which are multiple of 5, and then we test again with success and get this PR RTC so the original problem is fixed.
@richard67
Edit: The default value for Clear is not from the filter-form XML
I will revert the check for the values. It has no relation to a multiple of 5 as far as I can see.
A developer not using a standard value as default has to add it to the XML.
Like so:
append="33"
default="33"
I'm not sure yet that the start value 33 is accepted by the field type. That could be left to another issue.
Check for limit values removed!
Added to test instructions
append="33"
default="33"
@richard67
An idea: The start value problem could be related to the language string. In the example above with 33 the shown value is J33 as it's not translated. But keep it out of this PR.
It should be tested as it is now.
Ok, makes sense. Will test later today.
@schnuti I've tested again and think the behavior might be a bit confusing: After new installation plus your patch, when I go to the pligins list for the very first time, then then list limit is 20, even if in the XML during installation it was 10. When I then click the reset button, the list limit is the 10 from the XML. Is that desired or a mistake?
Do you agree that the value should come from the XML?
Unfortunaly - the first time it comes from the global list setting. When you send a search request (change a value or click the search button) the value comes from the XML. It's then also set to session.
At the moment I'm editing one of my own models to handle that.
I meant the value set in the limit field will be stored in the session.
I'll be back tomorrow - not more today.
Ok, have a nice evening.
I agree it should come from the XML.
A last thing for today.
That it works for ordering and direction is bacause you set default values for those in the model.
Try to change the XML with another default ordering.
i.e. one quick and dirty solution would be to set a default limit value in the model as well. I don't know if the parent model handle it without changes..
@richard67
I've found a work around for my own model that can't be an alternative for core.
In the populateState method I set a custom value
$app->setUserState('global.list.limit', 10);
and reset it afterwards
$app->setUserState('global.list.limit', $app->get('list_limit'));
Do we keep part two of this PR or should I revert it to use the default limit value from the global setting? Both can be confusing :(
Well is just my opinion: Maybe really better revert all additional parts of this PR so it fixes only the original problem. I wish someone else who has a better idea of how it was intended to work would comment here, too. I leave it up to you how you decide, just let me know later when ready for test again, and please keep the testing instructions up to date if you change something.
My opinion is to keep it but it doesn't matter. I'll use some override anyway. The new layout in J4 that clears ordering and limit leads to a lot more clicks when in list view. The J3 layout was better for me. The new backend multiplies the number of clicks needed in a work day anyway.
So shall this PR here tested like it is now? Or shall I wait for changes?
I think you have tested.
I'll make changes if it's decided.
this is a good example of why a PR should only address one issue
I have tested this item
I've tested all, including the extra test.
As the filter + sorting was reverted to the J3 solution this problem became obsolete.
Status | Pending | ⇒ | Closed |
Closed_Date | 0000-00-00 00:00:00 | ⇒ | 2019-08-31 07:48:21 |
Closed_By | ⇒ | schnuti |
@schnuti I applied this PR with patch tester on current 4.0-dev, then did
npm install
, but behavior is still the same as described in the issue. Did I do something wrong?