search

mathesar-foundation / mathesar

Web application providing an intuitive user experience to databases.
https://mathesar.org/
GNU General Public License v3.0
2.38k stars 330 forks source link

Frontend component: Pagination improvements #292

Closed pavish closed 2 years ago

pavish commented 3 years ago

The followed needs to be handled:

PRs that are related to this issue:

kgodey commented 3 years ago

@pavish I think you could repurpose this issue to cover virtual scrolling. Please update the issue.

pavish commented 3 years ago

@kgodey We have a reusable pagination component, which is part of the component library. We can have a separate issue for virtual scrolling.

kgodey commented 3 years ago

@pavish Since the infinite scrolling PR is already in review, I don't think there's a need to create an issue for it. If there's additional accessibility, documentation, or testing concerns, I think you're a better person to make issues for those than me.

agrawalanshul053 commented 2 years ago

I am trying to setup this repository in my local. I am using Windows OS. I have installed docker. I am facing difficulty in the following step. Clone the repository and then copy the .env.example file to .env like so: cp .env.example .env Please help me.

pavish commented 2 years ago

@agrawalanshul053 These commands are to copy files in a posix-like environment.

In windows, you can copy the .env.example to a new file and name it .env.

Alternatively, you can use your WSL (Windows subsystem for Linux) shell, and use posix-like commands.

silentninja commented 2 years ago

@agrawalanshul053 This issue is for the frontend component, I have moved this to a new issue here #830. Please use it for further discussion

mr-gabe49 commented 2 years ago

@pavish I'll take the documentation

pavish commented 2 years ago

Sure @mr-gabe49. Thanks!

mr-gabe49 commented 2 years ago

@pavish @seancolsen I would like some guidance on the behavior we are looking for on keyboard accessibility. I have seen different behaviors through the web:

Let me know your thoughts. Thanks!

pavish commented 2 years ago

@mr-gabe49 I prefer Left and Right arrow changing focused page and clicking enter would change current page. I would not use up and down since browsers by default use them for scrolling the webpage.

double click left arrow sets current page to the first page

I think this would be more confusing and we can avoid this for now.

eliminate left arrow when first page is active (same for right arrow and last page active).

We could disable the arrows instead of removing them entirely. Removing the arrow would change a shift in layout causing a sudden jerk.

I suggest having a look at ant.design pagination component as a reference.

Having said that, we could always do our own research and improve upon this. At the moment, I feel it would be better to follow accessibility patterns well established by other popular frameworks.

mr-gabe49 commented 2 years ago

@pavish Sounds good, thanks!

pavish commented 2 years ago

The pagination component is very similar to a navigation header, in terms of markup and usage. There are a few things we need to handle to ensure proper accessibility:

  1. The entire component should be enclosed in a nav which should contain an aria-label.
  2. The nav's aria-label should have a value "Pagination" by default and we should allow a prop to be passed for it, since a page could have multiple paginations and each should have their own distinct aria-label.
  3. I previously mentioned using left and right arrows to navigate but looking at aria standards and other component libraries, that does not appear to be common practice. Being able to switch focus between the pages with Tab would suffice.
    • Change the span elements for next, previous etc., to button elements. The browser would automatically then rely on the native :focus state to switch between the elements on tab, which would then be used announce the labels.
    • The dom elements should be disabled for any option that cannot be clicked. eg., clicking prev when page is 1.
  4. The selected page (not the focused page, just the selected page) should have an attribute aria-current="page".

References:

pavish commented 2 years ago

The following still needs to be handled as part of this issue:

We already have tests for paginationUtils as part of https://github.com/centerofci/mathesar/pull/925. We require tests for the DOM rendering aspect of the component.

seancolsen commented 2 years ago

I'm closing this because it's been open for almost a year. All that's left here is to add tests. While additional test coverage would be nice for this component, I don't see it being a priority. We have plenty of front end code without test coverage, and this code doesn't strike me as any more important than other areas of our codebase where we could seek to improve test coverage.