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

Feature/pagination actions #2108

Conversation

mrblippy
Copy link
Contributor

Checklist
Description of change

#1306

This PR adds in additional Paging options to the mui TablePagination component, allowing for navigating to the first and last page of search results

paginations

mrblippy added 5 commits July 24, 2020 12:20
By default, the MUI TablePagination component only allows for Previous/Next page navigation. Any further customisation must be implemented inside a separate component, which gets passed to the MUI ActionsComponent prop
Copy link
Contributor

@SammyIsConfused SammyIsConfused left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks Charlie! :)

Copy link
Member

@edalex-ian edalex-ian left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just some tweaks as per discussion and comments.

… reliable autotests, as well as giving oEQ admins more flexibility in custom ui styling
@PenghaiZhang
Copy link
Contributor

The Search page test is getting more and more test cases and mock data in. Almost all of tests are reliant on simulate which seems to be deprecated. So I am wondering we need to consider another approach...

@mrblippy
Copy link
Contributor Author

mrblippy commented Aug 5, 2020

The Search page test is getting more and more test cases and mock data in. Almost all of tests are reliant on simulate which seems to be deprecated. So I am wondering we need to consider another approach...

Yes. I think this test needs an overhaul in general. But in the interests of time we should revisit this in the future

Copy link
Contributor

@SammyIsConfused SammyIsConfused left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good 👍. I've highlighted a slight simplification but it will do the equivalent of what you did before so feel free to ignore.
Also I'm noting we talked about the issues with tests - that file will be rewritten in another ticket.

Copy link
Member

@edalex-ian edalex-ian left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good stuff dude! Love the new test.

Coupla quick tweaks and then you should be done. 👍

…ing purposes. And use it inside SearchPage and SearchPagination tests

Origianlly I tried exporting the function from the SearchPagination test. But eslint enforces the jest/no-export rule, so it had to go inside its own ts file
@mrblippy mrblippy requested a review from edalex-ian August 6, 2020 07:01
Copy link
Member

@edalex-ian edalex-ian left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's pretty well there, but in order or priority:

  1. Need JSDoc on your new utility function;
  2. Sorry about previous advice, looks like you've enough controls/elements here that you want an interface for your paginatorControls; and
  3. Should get the mocked data right.

@mrblippy mrblippy merged commit 0e5f243 into openequella:component/new-search-page Aug 9, 2020
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.

5 participants