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

Frontend component: Pagination improvements #292

Closed
3 of 4 tasks
pavish opened this issue Jun 28, 2021 · 14 comments
Closed
3 of 4 tasks

Frontend component: Pagination improvements #292

pavish opened this issue Jun 28, 2021 · 14 comments
Labels
affects: technical debt Improves the state of the codebase good first issue Everything in "Help wanted", PLUS being relatively easy and straightforward to implement. help wanted Community contributors can implement this ready Ready for implementation work: documentation Improvements or additions to documentation work: frontend Related to frontend code in the mathesar_ui directory

Comments

@pavish
Copy link
Member

pavish commented Jun 28, 2021

The followed needs to be handled:

  • Styling
  • ARIA, keyboard accessibility
  • Tests
  • Documentation

PRs that are related to this issue:

@pavish pavish added work: documentation Improvements or additions to documentation affects: technical debt Improves the state of the codebase work: frontend Related to frontend code in the mathesar_ui directory ready Ready for implementation labels Jun 28, 2021
@kgodey
Copy link
Contributor

kgodey commented Jul 14, 2021

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

@pavish
Copy link
Member Author

pavish commented Jul 14, 2021

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

@kgodey
Copy link
Contributor

kgodey commented Jul 16, 2021

@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.

@kgodey kgodey added good first issue Everything in "Help wanted", PLUS being relatively easy and straightforward to implement. help wanted Community contributors can implement this labels Jul 26, 2021
@agrawalanshul053
Copy link

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
Copy link
Member Author

pavish commented Nov 18, 2021

@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
Copy link
Contributor

@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
Copy link
Member

@pavish I'll take the documentation

@mr-gabe49 mr-gabe49 self-assigned this Dec 2, 2021
@pavish
Copy link
Member Author

pavish commented Dec 2, 2021

Sure @mr-gabe49. Thanks!

@mr-gabe49
Copy link
Member

@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:

  • Left/up and Right/down arrow change the current page
  • Left/up and Right/down arrow change the focused page, then clicking enter would change the current page
  • Other ideas:
    • double click left arrow sets current page to the first page
    • eliminate left arrow when first page is active (same for right arrow and last page active).

Let me know your thoughts. Thanks!

@pavish
Copy link
Member Author

pavish commented Jan 5, 2022

@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
Copy link
Member

@pavish Sounds good, thanks!

@pavish
Copy link
Member Author

pavish commented Jan 13, 2022

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:

@mr-gabe49 mr-gabe49 removed their assignment Feb 3, 2022
@pavish pavish changed the title Frontend component: Pagination Frontend component: Pagination improvements Feb 3, 2022
@pavish
Copy link
Member Author

pavish commented Feb 3, 2022

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

  • Write tests for the Pagination svelte component

We already have tests for paginationUtils as part of #925. We require tests for the DOM rendering aspect of the component.

@pavish pavish added ready Ready for implementation and removed status: started labels Feb 3, 2022
@kgodey kgodey modified the milestones: [06.2] 2022-04 improvements, [08.1] 2022-05 improvements May 2, 2022
@seancolsen
Copy link
Contributor

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
affects: technical debt Improves the state of the codebase good first issue Everything in "Help wanted", PLUS being relatively easy and straightforward to implement. help wanted Community contributors can implement this ready Ready for implementation work: documentation Improvements or additions to documentation work: frontend Related to frontend code in the mathesar_ui directory
Projects
No open projects
Development

No branches or pull requests

6 participants