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

Refactor/pagination a11y #516

Merged
merged 37 commits into from
Aug 27, 2021
Merged

Refactor/pagination a11y #516

merged 37 commits into from
Aug 27, 2021

Conversation

ChrisPaj
Copy link
Collaborator

  • @media screen and (max-width: 639px): width 100%
  • no doubled button borders
  • introduce prop 'small'
  • button:focus with visible outlines
  • adapt font size to 12px
  • introduce button:active state

@ChrisPaj ChrisPaj requested a review from acstll August 10, 2021 14:20
@ChrisPaj ChrisPaj requested a review from eeegor as a code owner August 10, 2021 14:20
@render
Copy link

render bot commented Aug 10, 2021

@acstll acstll requested a review from AnnaKuchtin August 11, 2021 06:40
Copy link
Collaborator

@acstll acstll left a comment

Choose a reason for hiding this comment

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

Thanks @ChrisPaj

I left some comments 🤓

Please make sure all checks pass (prettier is failing because of the index.html, but you can leave that one alone).

Also, we don't want to merge this in main yet, we need the a11y approval first. So we keep the branch, and keep it up-to-date with main so it's easier to merge when the time comes.

Copy link
Collaborator

@acstll acstll left a comment

Choose a reason for hiding this comment

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

Thank you @ChrisPaj, one more small thing 🙏

@acstll
Copy link
Collaborator

acstll commented Aug 11, 2021

@ChrisPaj the prettier check is not passing… it complaints about the index.html file, maybe you can try running yarn format once more?

@acstll acstll added the accessibility Related to accessibility label Aug 12, 2021
Copy link
Contributor

@AnnaKuchtin AnnaKuchtin left a comment

Choose a reason for hiding this comment

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

VirtualBox_MSEdge - Win10_12_08_2021_11_01_40
In HCM the icons are not being displayed anymore 🙈

@timheiler-ico
Copy link
Contributor

we are not using scale icons for next / prev?
Icon / Navigation / Right
Icon / Navigation / Left
would be great?

@timheiler-ico
Copy link
Contributor

timheiler-ico commented Aug 12, 2021

  • icons on mobile: 16x16 px (like desktop)
  • total mobile navigation height: 80px (40px top, 40px bottom part)

@acstll
Copy link
Collaborator

acstll commented Aug 13, 2021

we are not using scale icons for next / prev?
Icon / Navigation / Right
Icon / Navigation / Left
would be great?

I left them all inlined because they behave differently in HCM, would it be possible to add the other "double arrow" icons added to the library?

I did reduce the size of the inlined svgs to match the 16 size of the scale-icon icons…

@acstll
Copy link
Collaborator

acstll commented Aug 13, 2021

We're sending it out and closing for now until we get more feedback.

(The visual tests need to be updated!)

@acstll acstll closed this Aug 13, 2021
@acstll acstll reopened this Aug 27, 2021
@render
Copy link

render bot commented Aug 27, 2021

Copy link
Collaborator

@acstll acstll left a comment

Choose a reason for hiding this comment

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

The visual tests need to be updated.

@acstll acstll merged commit 586e0a3 into main Aug 27, 2021
@acstll acstll deleted the refactor/pagination-a11y branch August 27, 2021 11:13
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
accessibility Related to accessibility
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants