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

pr-type/bug-fix 7114 - wire up the page size event to alter page size #7124

Merged
merged 11 commits into from
Apr 10, 2024

Conversation

wouldd
Copy link
Contributor

@wouldd wouldd commented Mar 6, 2024

⚠️ Pre Checklist

Please complete ALL items in this checklist, and remove before submitting

  • I have read through the Contributing Documentation.
  • I have added relevant tests.
  • I have added relevant documentation.
  • I will add labels to the PR, such as pr-type/bug-fix, pr-type/feature-development, etc.

Summary

What does this PR do?
As per the ticket I raised #7114 this just wires up the existing pageSize value to the relevant event handler from the pagination library so that the page does change the number of items shown when a user selects them from the existing drop down box

Does this close any open issues?

Closes #7114

Screenshots

Include any relevant screenshots here.

Other Information

Any other information that is important to this PR.

@mintsweet
Copy link
Member

@Startrekzky there seems to be nothing wrong with this PR, but it only modifies the layout of one page, which will lead to differences in style from other pages. What do you think?

image

@wouldd
Copy link
Contributor Author

wouldd commented Mar 6, 2024

@mintsweet regards your comment about it modifying the layout of a page... are you saying that you did not previously see the page dropdown at all? because for me in chrome I always saw the page dropdown. it just did not do anything, hence I considered it a bug.

@mintsweet
Copy link
Member

mintsweet commented Mar 7, 2024

@wouldd Sorry, I understand that currently there is no operation to switch the page size, but it does not affect page turning. Why do you think this is an issue?

@wouldd
Copy link
Contributor Author

wouldd commented Mar 7, 2024

@mintsweet page turning is fine yes. but you can only associated config scopes with teh repos visibile on teh current page. selecting 'all' only selects those on the current page. so when the page size selector doesn't do anything you cannot have any other page size. So in my example where I linked ~800 repositories which I then needed to associate config scopes to. having to do that 10 at a time then manualy change pages is incredibly slow.
potentially it would ultimately be better to allow for a proper means to select all unmapped repos regardless of hte pages. but as quick and easy improvement. just wiring up the already present page size selector to actually change the page size means you can at least do this operation 100 at a time.

@mintsweet
Copy link
Member

@wouldd I see. Awesome PR, please wait patiently for another commiter's reply.

@wouldd
Copy link
Contributor Author

wouldd commented Mar 13, 2024

@mintsweet is there anyone specific that should be tagged for approving?

@mintsweet
Copy link
Member

@wouldd Sorry, we are preparing for 1.0, I will help you advance this PR, please be patient.

@Startrekzky
Copy link
Contributor

Hi @wouldd . Thanks for your contribution. I just reviewed the change and the PR itself is good. However, since the change will cause the inconsistency of the pagination between different pages, I'm not sure if we should merge it.

Let me know your thoughts on this. Thank you.

@wouldd
Copy link
Contributor Author

wouldd commented Apr 9, 2024

@Startrekzky when you say inconsistency, you mean it will only fix it on one page and not all pages? Do you want me to apply the same fix to other places?. It's worth noting that in the majority of instances of you redenering a table it is for things that you are unlikely to have that many of, so the impact of the broken page size is considerably less than here where you can easily have 1000 entries.

@Startrekzky
Copy link
Contributor

@wouldd I agree that some lists will have more entries than the others, and your improvement is good. The question was whether we should merge it now or merge it when all similar pages are updated accordingly.

We can merge it now, but I suggest you update the project list's pagination later. Does it make sense?

Cc. @mintsweet

@mintsweet mintsweet merged commit 9589a6a into apache:main Apr 10, 2024
7 of 8 checks passed
abeizn pushed a commit that referenced this pull request Apr 12, 2024
…#7124)

* 7114 - wire up the page size event to alter page size

* rollback bad merge change

---------

Co-authored-by: Daniel Would <daniel.would@oribs.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[Feature][config-ui] Connections pagination isn't wired up to page size drop down
3 participants