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

[#173778636] UI accessibility rework #2059

Merged
merged 11 commits into from
Jul 17, 2020

Conversation

fabriziofff
Copy link
Contributor

Short description:
This pr introduces some accessibility rework.

List of changes proposed in this pull request:

  • Change the height of the standard button to 40 dp.

Schermata 2020-07-16 alle 16 55 21

  • Extend the touchable height for all the buttons to 48 dp.

Schermata 2020-07-16 alle 16 57 35

How to test:
Describe the tests that you ran to verify your changes. Provide instructions so we can reproduce.

@fabriziofff fabriziofff added the a11y Activities related to accessibility label Jul 16, 2020
@pagopa-github-bot
Copy link
Collaborator

pagopa-github-bot commented Jul 16, 2020

Affected stories

  • 🐞 #173778636: Migliorare l'accessibilità della UI di IO

Generated by 🚫 dangerJS

@fabriziofff fabriziofff marked this pull request as ready for review July 16, 2020 15:35
*
* @param height
*/
const calculateSlop = (height: number): number => {
Copy link
Contributor

Choose a reason for hiding this comment

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

Since this function may be reused on other components what do you think of isolating it in a util such as utils/ui.ts?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

My idea was to remove all the code inside this file as soon as will be a refactoring of the basic components.
Perhaps it would be better to leave it there to facilitate its removal and if it should serve elsewhere, move it to a utility file. What you think?

Copy link
Contributor

Choose a reason for hiding this comment

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

My idea was to remove all the code inside this file as soon as will be a refactoring of the basic components.
Perhaps it would be better to leave it there to facilitate its removal and if it should serve elsewhere, move it to a utility file. What you think?

I'm agree.
Until we don't refactor these partes it's convenient to add code in a focused otherwise it will be more difficult to remove/refactor if it is spread around the project

*
* @param height
*/
const calculateSlop = (height: number): number => {
Copy link
Contributor

Choose a reason for hiding this comment

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

To avoid compute slop on each button spawn, could we compute that values only a time?

const smallSlop = calculateSlop(customVariables.btnSmallHeight);
const xsmallSlop = calculateSlop(customVariables.btnXSmallHeight);
const dafaultSlop = calculateSlop(customVariables.btnHeight);
/**
 * This is a temporary solution to extend the touchable area using the existing theme system.
 * @deprecated
 * @param props
 */
const getSlopForCurrentButton = (props: Props) => {
  if (props.small) {
    return smallSlop;
  } else if (props.xsmall) {
    return xsmallSlop;
  }
  return dafaultSlop;
};

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yep, definitely better :D changed in 640746a

@fabriziofff fabriziofff requested a review from Undermaken July 17, 2020 12:55
@Undermaken Undermaken merged commit 371efc8 into master Jul 17, 2020
@CrisTofani CrisTofani deleted the 173778636-ui-accessibility-rework branch March 23, 2021 11:09
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
a11y Activities related to accessibility
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants