Show product collection limiter only if makes sense

[fballiano]

In the adminhtml's configuration section you can decide how many products per page can be shown, and allow the visitors to select between some different "options" (default is 12 products, 24 products, 36 products).
But if you configure to allow only 1 possibility (say 12 products and that's it) the visitors are shown the select but the select contains only one value.

This PR removed the select (sparing a few DOM elements) if the visitors wouldn't be able to change the value.

I know it doesn't look amazing in the default rwd style but that will be addressed in another PR.

[kiatng]

I do not see the benefit of not using getShowPerPage():

magento-lts/app/code/core/Mage/Page/Block/Html/Pager.php

Lines 184 to 190 in 76bae08

	public function getShowPerPage()
	{
	if (count($this->getAvailableLimit()) <= 1) {
	return false;
	}
	return $this->_showPerPage;
	}

[fballiano]

I do not see the benefit of not using getShowPerPage():

there's no need to call getAvailableLimit() 2 times (one inside getShowPerPage() and one a few lines later)

[kiatng]

there's no need to call getAvailableLimit() 2 times (one inside getShowPerPage() and one a few lines later)

I know, but it just returns a constant array. So the real benefit is questionable. Also, it looks weird if the function is not used anymore. Should it be deprecated?

[fballiano]

getShowPerPage() is not even available in all the blocks that have getAvailableLimit(), for uniformity I used the same syntax on all the places that I modified.

we could deprecated it but I don't think there's a real need for that, I don't have on opinion on that.

[sreichel]

I'd go a step further and also check if collection size is smaller then lowest items per page.

[fballiano]

@sreichel implemented ;-)

[addison74]

I think several scenarios need to be evaluated. For example, we have 12, 24, 36 in the list.

(if products <= 12) - the selection is not displayed
(if products > 12 and products <= 24) - the selection with 12 and 24 is displayed
(if products > 24 and products <= 36) - the selection with 12, 24 and 36 is displayed

[fballiano]

I hoped I could get away with a simpler implementation ahhahah I'll look into that

[addison74]

The situation is interesting and needs a deep investigation. In some pages the selection will be missing because the number of products in that category fulfills a condition but in others it exists. We should avoid having a visual inconsistency, I kept looking at the websites of some important stores and I didn't see the selection removed, regardless of the number of products in the list.

If the solution is to remove certain values from the list, then the first value must be maintained, so the selector must not be hidden. Depending on the number of products, the following interval will be added. Let's not end up complicating things unnecessarily.

Even if we want to reinvent the wheel, let's say if I have 6 products on a page and 12, 24, 36 in the selection, even if I choose to display 36, I am informed that only 6 products are displayed.

[fballiano]

mmmm so what should I do? revert to the first version of this PR? which didn't create any GUI inconsistency?

[addison74]

Personally I would not make any changes in the code for now. I would keep the idea, I would discuss it and if a final shape is reached, I would implement it. it can also be drafted. I'm sorry to send you back, but I didn't test the PR and looking over it I noticed these issues.

The selection must exist, when the visitors will notice its absence, not everyone is smart enough to understand that there are less than X products on the page and for that reason it is missing. Maybe for this reason the big stores leave the selection permanently. In addition, if the values are large, let's say 50 and 100, either the selection appears or not for a single value, which is very strange.

[fballiano]

exactly, my original PR doesn't create any inconsistency because it's based on a global configuration set by the store manager, not on the amount of products. if the store manager sets to show only X products and no selection, for the whole website, I don't see any reason why the selector should be visible with a single element.

[fballiano]

I've reverted the PR to my original code (+ the cosmetic changes by @sreichel), which doesn't create any GUI inconsistency and could be merged safely already.

further improvement can be discussed in a new discussion if we want (but, to avoid inconsistencies) I'd leave it as it is.

[addison74]

The problem arises when the first value in the list is a big one, for example 36 or 48. Scrolling the screen on a smartphone and seeing a long list of products the visitor will be tempted to look for the selection. He won't find it because there is a condition. Hence the confusion that will be created, some pages have selection, others don't.

This is the website from which I buy my outdoor products. Please visit this category: https://www.rei.com/c/mens-swim-caps. It has the VIEW options set to the values 30 and 90. They have not been removed the container, although there are 3 products on the page. The selection is part of the layout of the page and they did not give it up precisely to maintain consistency and avoid confusion.

I am really sorry I have a different opinion related to this PR.

[fballiano]

That's what I'm saying, if you as store manager had configured openmage to have only 30 and removed the "90", why should you have a selector that doesn't allow any selection?

EDIT: again, I'm not talking about the number of products in the category, that was not part of the original version of the PR.

[addison74]

If there are still more of you who want to implement this change, please add an option in the config to Enable/Disable this feature.

[fballiano]

mmm no sorry I don't agree on having everything as an option, I'll leave it as it is.

Also, that option should appear only when the "available limits" contains only one value, which is not even possible for us since the "available limits" is a text field.

[addison74]

In this case, without the possibility of disabling this feature in the Backend, I will let others approve the PR.

[fballiano]

Thanks absolutely fine

[justinbeaty]

I agree with the change how it is. The extended version where we potentially disable it per page is a part of a longer discussion as there are clearly differing opinions, but this version is simple and straightforward.

Hence the confusion that will be created, some pages have selection, others don't.

@addison74 I think you're misunderstanding what the PR does with the reverted changes. The selection will either always be on every page, or be on no page at all. What @fballiano is doing is hiding the selection if there would only be one option (i.e. 30) then the selection is useless, because you can't change it.

[addison74]

@justinbeaty - I understood very well what this PR proposes. but I do not agree with it as long as there is no option to disable the feature. Maybe there are stores where the administrators want that selection to be visible all the time. What do we do in this case? Do we force them to accept this change and then look for how to remove it from the code? However, that selection was there for 14 years and I think everyone got used to it. Here is issue, I accept this PR with a config option, but the author does not want to include it in Backend in Config > Catalog > Frontend section.

[justinbeaty]

@justinbeaty - I understood very well what this PR proposes. but I do not agree with it as long as there is no option to disable the feature. Maybe there are stores where the administrators want that selection to be visible all the time. What do we do in this case? Do we force them to accept this change and then look for how to remove it from the code? However, that selection was there for 14 years and I think everyone got used to it. Here is issue, I accept this PR with an config option, but the author does not want to include it in Backend in Config > Catalog > Frontend section.

I guess the point is that there is a config option to control this. To make sure the selection box shows up, you simply need to give at least two options here:

By default, it will be set to the original behavior. I would guess that if a store admin specifically set this value to only have one option, they probably already modified the phtml file to hide the selection box.

[addison74]

@justinbeaty - Several successive changes occurred and I have to test this PR.

If there is only one value in that list, it is obvious that the selection should no longer appear, but it will no longer appear everywhere, regardless of whether there are 1 or 1000 products on a category page.

From what was discussed, I was left with the impression that the selection is hidden if a condition is met, i.e. in some categories it is displayed in others not, compared to the number of products.

[fballiano]

I’ve reverted those changes:
#2502 (comment)

[sreichel]

Sorry for my suggestion and bringing confusion.

[addison74]

I tested this PR and according to the last modification it corresponds to its description. Under these conditions I have no reason not to approve it.

This is result in Frontend for the multi-valued version:

This is the result in Frontend for a single value version:

[addison74]

In my previous post I presented the test for the Grid mode, but I omitted to do it for the List mode as well. I am attaching screenshots, this PR applies correctly for both modes.

[fballiano]

Thanks guys 😊

[github-actions]

Unit Test Results

1 files  ±0  1 suites  ±0   0s ⏱️ ±0s
0 tests ±0  0 ✔️ ±0  0 💤 ±0  0 ❌ ±0 
7 runs  ±0  5 ✔️ ±0  2 💤 ±0  0 ❌ ±0 

Results for commit 988e9ff. ± Comparison against base commit b6bb0ed.