NPM Resource Changed ? Pending

User tests: Successful: Unsuccessful:

avatar schnuti
schnuti
23 Jul 2019

Pull Request for Issue #21676.

Summary of Changes

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.

Testing Instructions

Before installing this fix:

  • Go to the Plugin List or any other list with more than ca 30 items.
  • Check the list limit, probably 20 or 25.
  • Click on the Clear button.
  • All items will appear in the list and with a wrong list limit.
    After patch install and JS rebuild.
  • Do the same.
  • The number of items is according to the filter form setting.
  • The list limit is set to the filter form setting
    Difference check
  • Edit the default value for list limit in the filter form
    e.g. in administrator/components/com_plugins/forms/filter_plugins.xml
    Change line 78 to default="5"
    -> If you use a non standard value e.g. edit to
    append="33"
    default="33"

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.

Actual result

Shows all items.

Documentation Changes Required

No

avatar schnuti schnuti - open - 23 Jul 2019
avatar schnuti schnuti - change - 23 Jul 2019
Status New Pending
avatar joomla-cms-bot joomla-cms-bot - change - 23 Jul 2019
Category JavaScript Repository NPM Change
avatar schnuti schnuti - change - 23 Jul 2019
The description was changed
avatar schnuti schnuti - edited - 23 Jul 2019
95e300b 23 Jul 2019 avatar schnuti Hound
avatar schnuti schnuti - change - 23 Jul 2019
Labels Added: NPM Resource Changed ?
avatar richard67
richard67 - comment - 23 Jul 2019

@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?

avatar schnuti
schnuti - comment - 23 Jul 2019

@richard67 In what view did you test? I'll test that myself.

avatar richard67
richard67 - comment - 23 Jul 2019

@schnuti In the plugins view. With the change I've just suggested it works.

avatar richard67
richard67 - comment - 23 Jul 2019

I have tested this item successfully on 5cc29eb


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

avatar richard67 richard67 - test_item - 23 Jul 2019 - Tested successfully
avatar schnuti
schnuti - comment - 23 Jul 2019

@richard67 Thanks!

avatar richard67
richard67 - comment - 23 Jul 2019

@schnuti Thanks for the PR.

avatar franz-wohlkoenig franz-wohlkoenig - change - 24 Jul 2019
Title
[4.0] Searchtools problems Fix for #21676
[4.0] Searchtools problems Fix
avatar franz-wohlkoenig franz-wohlkoenig - edited - 24 Jul 2019
avatar schnuti
schnuti - comment - 24 Jul 2019

@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.

avatar dgrammatiko
dgrammatiko - comment - 24 Jul 2019

@schnuti unfortunately I cannot answer this question without looking at all the possible implementations

avatar richard67
richard67 - comment - 24 Jul 2019

@schnuti Is it ready for test?

avatar schnuti
schnuti - comment - 24 Jul 2019

@richard67 and all
Ready for test. jQuery was removed after your test. That should not change anything.

avatar richard67
richard67 - comment - 24 Jul 2019

@schnuti I've tested and it changed something: The PR does not fix the issue anymore, behavior with and without PR is the same. Tested as before with applying patch, running npm install, testing in plugins list.

avatar richard67
richard67 - comment - 24 Jul 2019

@schnuti Sorry, false alarm. Forget my previous comment, all ok.

avatar richard67
richard67 - comment - 24 Jul 2019

I have tested this item successfully on 3722b41


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

avatar richard67 richard67 - test_item - 24 Jul 2019 - Tested successfully
avatar schnuti
schnuti - comment - 25 Jul 2019

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.

avatar richard67
richard67 - comment - 25 Jul 2019

@schnuti Did you ask me in your previous comment? If so, I have no idea what you mean.

avatar schnuti
schnuti - comment - 25 Jul 2019

@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.)

avatar richard67
richard67 - comment - 25 Jul 2019

@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.

avatar schnuti
schnuti - comment - 25 Jul 2019

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.

avatar richard67
richard67 - comment - 25 Jul 2019

Let us know when ready so we don't test while still in progress.

avatar richard67
richard67 - comment - 26 Jul 2019

@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 ?

avatar schnuti
schnuti - comment - 26 Jul 2019

@richard67
You'll have it in a few hours. The extra tests worked fine for me.

avatar joomla-cms-bot joomla-cms-bot - change - 26 Jul 2019
Category JavaScript Repository NPM Change JavaScript Repository NPM Change Layout
avatar schnuti
schnuti - comment - 26 Jul 2019

Ready for test if the checks go through.
Do you need some test instructions?

avatar richard67
richard67 - comment - 26 Jul 2019

Do you need some test instructions?

Would be helpful, yes.

avatar schnuti schnuti - change - 26 Jul 2019
The description was changed
avatar schnuti schnuti - edited - 26 Jul 2019
avatar schnuti
schnuti - comment - 26 Jul 2019

Test instructions added on the top.

avatar richard67
richard67 - comment - 26 Jul 2019

I will test tomorrow, today am busy for the rest of the day.

avatar richard67
richard67 - comment - 26 Jul 2019

@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?

avatar schnuti
schnuti - comment - 26 Jul 2019

@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.

avatar richard67
richard67 - comment - 26 Jul 2019

@schnuti When I clear. I think (not tested yet) it is the same without your PR that values must be multiple of 5. Your PR only makes it possible to change the xalue in the xml. So I would also accept if you say you don't fix it.

avatar schnuti
schnuti - comment - 26 Jul 2019

Without my PR part 2 the XML value is not used as default in the Javascript.

avatar richard67
richard67 - comment - 26 Jul 2019

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.

avatar schnuti
schnuti - comment - 26 Jul 2019

Maybee try to set the Global list limit to 13 without PR II.

avatar richard67
richard67 - comment - 26 Jul 2019

I will try tomorrow.

avatar schnuti
schnuti - comment - 26 Jul 2019

@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?

avatar richard67
richard67 - comment - 26 Jul 2019

I also don't know exactly how to do that. No idea if it is necessary or worth it. Will test tomorrow. Good night.

avatar richard67
richard67 - comment - 27 Jul 2019

@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.

avatar schnuti
schnuti - comment - 27 Jul 2019

@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.

avatar schnuti
schnuti - comment - 27 Jul 2019

Check for limit values removed!
Added to test instructions
append="33"
default="33"

avatar schnuti schnuti - change - 27 Jul 2019
The description was changed
avatar schnuti schnuti - edited - 27 Jul 2019
avatar schnuti
schnuti - comment - 27 Jul 2019

@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.

avatar richard67
richard67 - comment - 27 Jul 2019

Ok, makes sense. Will test later today.

avatar richard67
richard67 - comment - 27 Jul 2019

@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?

avatar schnuti
schnuti - comment - 27 Jul 2019

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.

avatar schnuti
schnuti - comment - 27 Jul 2019

I meant the value set in the limit field will be stored in the session.
I'll be back tomorrow - not more today.

avatar richard67
richard67 - comment - 27 Jul 2019

Ok, have a nice evening.

avatar richard67
richard67 - comment - 27 Jul 2019

I agree it should come from the XML.

avatar schnuti
schnuti - comment - 27 Jul 2019

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..

avatar schnuti
schnuti - comment - 28 Jul 2019

@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 :(

avatar richard67
richard67 - comment - 28 Jul 2019

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.

avatar schnuti
schnuti - comment - 28 Jul 2019

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.

avatar richard67
richard67 - comment - 28 Jul 2019

So shall this PR here tested like it is now? Or shall I wait for changes?

avatar schnuti
schnuti - comment - 28 Jul 2019

I think you have tested.
I'll make changes if it's decided.

avatar brianteeman
brianteeman - comment - 28 Jul 2019

this is a good example of why a PR should only address one issue

avatar richard67
richard67 - comment - 28 Jul 2019

I have tested this item successfully on a767cdd


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

avatar richard67 richard67 - test_item - 28 Jul 2019 - Tested successfully
avatar richard67
richard67 - comment - 28 Jul 2019

I've tested all, including the extra test.


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

avatar schnuti
schnuti - comment - 31 Aug 2019

As the filter + sorting was reverted to the J3 solution this problem became obsolete.

avatar schnuti schnuti - change - 31 Aug 2019
Status Pending Closed
Closed_Date 0000-00-00 00:00:00 2019-08-31 07:48:21
Closed_By schnuti
avatar schnuti schnuti - close - 31 Aug 2019

Add a Comment

Login with GitHub to post a comment