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

feat(VPagination): add a11y support #9926

Merged
merged 3 commits into from Mar 23, 2020
Merged

feat(VPagination): add a11y support #9926

merged 3 commits into from Mar 23, 2020

Conversation

abnersajr
Copy link
Contributor

Description

This PR adds aria-* attributes improving accessibility of the pagination component.
Another feature is the component localization with translations to all available locations.

Motivation and Context

It fixes the issue #9849 following all the instructions requested.

How Has This Been Tested?

I manually tested and also the unit test was fixed to work with the new changes.

Markup:

<template>
  <v-container>
    <v-row justify="center">
        <v-col cols="8">
          <v-container class="max-width">
            <v-pagination
              v-model="page"
              class="my-4"
              :length="15"
            ></v-pagination>
          </v-container>
        </v-col>
      </v-row>
  </v-container>
</template>

<script>
export default {
  data: () => ({
    page: 1,
  })
}
</script>

Types of changes

  • Improvement/refactoring (non-breaking change that doesn't add any features but makes things better)

Checklist:

  • The PR title is no longer than 64 characters.
  • The PR is submitted to the correct branch (master for bug fixes and documentation updates, dev for new features and breaking changes).
  • My code follows the code style of this project.

@jacekkarczmarczyk jacekkarczmarczyk added C: VPagination VPagination T: enhancement Functionality that enhances existing features labels Dec 10, 2019
Copy link
Member

@jacekkarczmarczyk jacekkarczmarczyk left a comment

Choose a reason for hiding this comment

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

LGTM except some translations

packages/vuetify/src/locale/en.ts Show resolved Hide resolved
packages/vuetify/src/locale/en.ts Show resolved Hide resolved
packages/vuetify/src/locale/en.ts Show resolved Hide resolved
packages/vuetify/src/locale/pl.ts Show resolved Hide resolved
Copy link
Member

@johnleider johnleider left a comment

Choose a reason for hiding this comment

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

A few comments, otherwise LGTM.

@johnleider johnleider changed the title improvement(vpagination): adds a11y in VPagination feat(VPagination): add a11y support Dec 17, 2019
@johnleider johnleider changed the base branch from master to dev December 17, 2019 16:02
@johnleider
Copy link
Member

I've moved this to the dev branch since it's a feature.

@johnleider johnleider added this to the v2.2.0 milestone Dec 17, 2019
@abnersajr
Copy link
Contributor Author

@johnleider I think in case we decide to go as props I think this will lose the idea of be localized.
What I can consider is make the change through props as an alternative. And let the localized as default.
I would prefer make this change in another PR. Even it's nothing big, but to be separated the ideas.
What you think?

@jacekkarczmarczyk
Copy link
Member

@abnersajr prop can have the default value pointing to the locales, this will give possibility to change the value globally (in locales) or per component (in prop). See for example https://github.com/vuetifyjs/vuetify/blob/master/packages/vuetify/src/components/VAlert/VAlert.ts#L44

@johnleider johnleider removed this from the v2.2.0 milestone Jan 10, 2020
@johnleider
Copy link
Member

@abnersajr pulse check on this.

@abnersajr
Copy link
Contributor Author

@johnleider sorry. Things are going crazy this year begin. I will solve it by tomorrow.👊

@abnersajr
Copy link
Contributor Author

@johnleider @jacekkarczmarczyk all comments done.

@TravisBuddy
Copy link

TravisBuddy commented Jan 22, 2020

Hey @abnersajr,
Your changes look good to me!

View build log

TravisBuddy Request Identifier: 7e12be80-3d7f-11ea-bf94-57f545f30904

@abnersajr
Copy link
Contributor Author

abnersajr commented Jan 23, 2020

I made a mess with the commits, I'm trying to solve this. In case someone have a suggestion please let me know.
EDIT: Fixed messy commits

@johnleider
Copy link
Member

Is this non-breaking? If so, I believe we can get it in as a bug fix seeing as it pertains to a11y.

@johnleider johnleider merged commit 042a4bd into vuetifyjs:dev Mar 23, 2020
@johnleider johnleider added this to the v2.3.0 milestone Mar 23, 2020
@lock lock bot locked as resolved and limited conversation to collaborators Apr 25, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
C: VPagination VPagination T: enhancement Functionality that enhances existing features
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants