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

Please remove "required" from ngb-pagination.page input #4518

Closed
e-oz opened this issue May 31, 2023 · 7 comments
Closed

Please remove "required" from ngb-pagination.page input #4518

e-oz opened this issue May 31, 2023 · 7 comments

Comments

@e-oz
Copy link

e-oz commented May 31, 2023

You don't really need it to be provided - you even has a default value.

Versions of Angular, ng-bootstrap and Bootstrap:

Angular: 16

ng-bootstrap: 15

Bootstrap: 5.3

@jnizet
Copy link
Member

jnizet commented May 31, 2023

Could you please explain why you think this input shouldn't be required?

It seems completely illogical to me to display a paginnation component without telling it which page is the current one. Sure, it can use a default value, but the goal of required: true is to avoid bugs, i.e. to warn the developers when they use a component in a way that doesn't make sense. And to me, it doesn't make much sense using a pagination component without telling it what page is the current one.

Maybe I'm missing something, but what?

@e-oz
Copy link
Author

e-oz commented May 31, 2023

This component works just fine without this input. There is a default value exactly because of this fact.

@jnizet
Copy link
Member

jnizet commented May 31, 2023

Sure, the component doesn't crash if you don't give it the page to display. But in which actual situation is it not a bug to display a page without telling what the page is? Is there any real case where you found it useful to display a pagination without telling it which page to display?
If you don't answer that question, there's no good reason for us to consider your issue.

@e-oz
Copy link
Author

e-oz commented May 31, 2023

Well, with that hostile tone, I’m absolutely fine if you will not consider this issue. You are making an input required which is not actually required - that’s an obvious mistake.

In my opinion (and from my experience), binding to a variable using [(page)] might lead to data race, and it's much safer to use selectPage() and (pageChange).

@jnizet
Copy link
Member

jnizet commented May 31, 2023

Sorry if my tone sounded hostile, that wasn't my intention.

I don't think it's obviously a mistake for the reason I explained already: the purpose of required is to prevent mistakes in the usage of the component, and to me, not providing a page is a mistake.

If you think there is an issue with binding the page the documented way, then this should be, IMHO, the subject of a separate issue explaining what the problem is.

And the selectPage() method you mention is not a documented method of the component.

@maxokorokov maxokorokov added this to the 15.0.1 milestone Jun 5, 2023
@maxokorokov
Copy link
Member

maxokorokov commented Jun 5, 2023

Technically, I can find use cases where the [page] can be be optional indeed. Pagination handles page selection/adjustment from the inside perfectly fine and if you don't care which page to pre-select - you don't need it.

To me it is like rating that is pre-set to 0 automatically. Here the page is pre-set to 1. User can still click and change the page.

I think the very minimum should be <ngb-pagination [collectionSize] />, so it would give you some non-empty display without looking at documentation.

Another problem here that is was actually a breaking change that was not documented. You could use pagination without [page], now you can't.

@jnizet, do you agree with my "pre-select" reasoning? do you think we could rollback the required on the [page] along with others in your other PR ?

jnizet added a commit to jnizet/ng-bootstrap that referenced this issue Jun 5, 2023
This allows setting them in directives composed with the ng-bootstrap ones, until issue angular/angular#50510 of Angular is fixed.

fix ng-bootstrap#4516
fix ng-bootstrap#4518
@jnizet
Copy link
Member

jnizet commented Jun 5, 2023

Sure @maxokorokov

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

3 participants