Fix a11y issue with Data Science Projects: Pagination #995

jenny-s51 · 2023-03-07T21:22:02Z

Towards #965 , specifically DS Projects page: "1-n Projects added"

- The only changes in this PR are related to the pagination component within Table.tsx. 
- Please only review the pagination updates in Table.tsx.

How Has This Been Tested?

Data Science Projects List - before:

Data Science Projects List - after:

Test Impact

Request review criteria:

The commits have meaningful messages (squashes happen on merge by the bot).
Included any necessary screenshots or gifs if it was a UI change.
Included tags to the UX team if it was a UI/UX change.
Testing instructions have been added in the PR body (for PRs involving changes that are not immediately obvious).
The developer has manually tested the changes and verified that the changes work
The developer has added tests or explained why testing cannot be added (unit tests & storybook for related changes)

Gkrumbach07 · 2023-03-07T23:06:00Z

pagination changes look good.

Are you going to rebase this pr? or will it rebase when we merge?

DaoDaoNoCode

Pagination changes lgtm, add variant could change the ID of the pagination components and differentiate the top and bottom ones.

lucferbux

Pagination seems great.

frontend/src/components/Table.tsx

andrewballantyne · 2023-03-08T17:24:19Z

...end/src/pages/modelServing/screens/projects/ServingRuntimeModal/ServingRuntimeTokenInput.tsx

@@ -45,6 +45,7 @@ const ServingRuntimeTokenInput: React.FC<ServingRuntimeTokenInputProps> = ({
  return (
    <FormGroup
      label="Service account name"
+      fieldId="simple-form-name-01"


Perhaps not this PR / a11y related -- but simple-form? @lucferbux a little help here? Why is this not properly named? This extends throughout the component.

This fieldId of FormGroup needs to match the id of TextInput for VO to call out the form labels correctly! Agreed that we could rename it to something that makes more sense - updated here, and in 26c978e#diff-ee0ec36a5925e8c06ac689a1cbec0356d48cb73fe36577c2c8e9046d6d6583e7

andrewballantyne · 2023-03-08T17:27:51Z

frontend/src/pages/projects/components/PasswordInput.tsx

@@ -8,7 +8,11 @@ const PasswordInput: React.FC<React.ComponentProps<typeof TextInput>> = (props)
  return (
    <InputGroup>
      <TextInput {...props} type={isPassword ? 'password' : 'text'} />
-      <Button variant="control" onClick={() => setPassword(!isPassword)}>
+      <Button
+        aria-label={isPassword ? 'Show key' : 'Hide key'}


hmmm -- maybe this should be something like Show secret, Show text, Show hidden text, or Show password? Key seems unclear. Not sure if I would be able to understand that if I just heard a screen reader show me. Thoughts?

Agreed - updated to show/hide password which would make sense as this component is called PasswordInput.

andrewballantyne · 2023-03-08T17:31:57Z

frontend/src/pages/projects/screens/projects/EmptyProjects.tsx

          href="/notebookController"
+          component="a"
          variant="link"
          onClick={(e) => {
            e.preventDefault();


I wonder if this should not be more like the companion button on the state-filled page.

Great point - updated in 26c978e#diff-047fef1a71926288a0e0ce5836ce60d08cdb3656469356b77cb1c2609ef958cd

andrewballantyne · 2023-03-08T17:34:06Z

frontend/src/pages/projects/screens/spawner/environmentVariables/EnvTypeSelectField.tsx

@@ -36,6 +36,7 @@ const EnvTypeSelectField: React.FC<EnvTypeSelectFieldProps> = ({
              onToggle={() => setOpen(!open)}
              selections={envVariable.type || ''}
              placeholderText="Select one"
+              aria-label="Select one"


Would this not be Select environment variable type? I get the UI doesn't say that -- not sure how much "help" we should grant to screen readers.

How much does an aria-label need to cover "description" of what is happening rather than echoing the text I assume that would be already read by the screen reader (I have not used a screen reader to test this).

The text should be updated upon rebasing https://github.com/opendatahub-io/odh-dashboard/pull/994/files.

I tested with screenreader, and it seems like PF has some issues with the current select component and the aria-label/placeholder text doesn't get called out by VO. However this will be resolved in the next major release as PF is introducing a new Select component that has the correct VO behavior @andrewballantyne

andrewballantyne · 2023-03-08T17:35:52Z

I suspect I ended up commenting on things in the other PRs... apologizes for that.

andrewballantyne

Looks great! Thanks for all your hard work.

openshift-ci · 2023-03-09T18:59:59Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: andrewballantyne, DaoDaoNoCode, lucferbux

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

~~OWNERS~~ [andrewballantyne]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

openshift-ci · 2023-03-09T19:30:34Z

New changes are detected. LGTM label has been removed.

)

openshift-ci bot requested review from DaoDaoNoCode and Gkrumbach07 March 7, 2023 21:22

DaoDaoNoCode approved these changes Mar 8, 2023

View reviewed changes

openshift-ci bot assigned DaoDaoNoCode Mar 8, 2023

openshift-ci bot added the lgtm label Mar 8, 2023

lucferbux approved these changes Mar 8, 2023

View reviewed changes

openshift-ci bot assigned lucferbux Mar 8, 2023

andrewballantyne requested changes Mar 8, 2023

View reviewed changes

openshift-ci bot assigned andrewballantyne Mar 8, 2023

openshift-ci bot removed the lgtm label Mar 8, 2023

openshift-merge-robot added the needs-rebase PR needs to be rebased label Mar 8, 2023

jenny-s51 force-pushed the pagination_a11y branch from 02efe22 to c9a0c70 Compare March 9, 2023 18:56

fix pagination a11y errors and remaining PR comments

23f6950

jenny-s51 force-pushed the pagination_a11y branch from c9a0c70 to 23f6950 Compare March 9, 2023 18:58

openshift-merge-robot removed the needs-rebase PR needs to be rebased label Mar 9, 2023

andrewballantyne approved these changes Mar 9, 2023

View reviewed changes

openshift-ci bot added the lgtm label Mar 9, 2023

openshift-ci bot added the approved label Mar 9, 2023

andrewballantyne linked an issue Mar 9, 2023 that may be closed by this pull request

[A11y] Address A11y issues on Data Science Projects #965

Closed

Merge branch 'main' into pagination_a11y

91113ef

openshift-ci bot removed the lgtm label Mar 9, 2023

andrewballantyne added the lgtm label Mar 9, 2023

openshift-merge-robot merged commit 3b5c7b8 into opendatahub-io:main Mar 9, 2023

lucferbux pushed a commit to lucferbux/odh-dashboard that referenced this pull request Mar 13, 2023

fix pagination a11y errors and remaining PR comments (opendatahub-io#995

cbb4a93

)

bartoszmajsak pushed a commit to maistra/odh-dashboard that referenced this pull request Mar 30, 2023

fix pagination a11y errors and remaining PR comments (opendatahub-io#995

63c0e4f

)

strangiato pushed a commit to strangiato/odh-dashboard that referenced this pull request Oct 18, 2023

fix pagination a11y errors and remaining PR comments (opendatahub-io#995

1f2f3d7

)

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix a11y issue with Data Science Projects: Pagination #995

Fix a11y issue with Data Science Projects: Pagination #995

jenny-s51 commented Mar 7, 2023 •

edited

Loading

Gkrumbach07 commented Mar 7, 2023 •

edited

Loading

DaoDaoNoCode left a comment

lucferbux left a comment

andrewballantyne Mar 8, 2023

jenny-s51 Mar 8, 2023 •

edited

Loading

andrewballantyne Mar 8, 2023

jenny-s51 Mar 9, 2023

andrewballantyne Mar 8, 2023

jenny-s51 Mar 8, 2023

andrewballantyne Mar 8, 2023

jenny-s51 Mar 9, 2023 •

edited

Loading

andrewballantyne commented Mar 8, 2023

andrewballantyne left a comment

openshift-ci bot commented Mar 9, 2023

openshift-ci bot commented Mar 9, 2023

Fix a11y issue with Data Science Projects: Pagination #995

Fix a11y issue with Data Science Projects: Pagination #995

Conversation

jenny-s51 commented Mar 7, 2023 • edited Loading

How Has This Been Tested?

Test Impact

Request review criteria:

Gkrumbach07 commented Mar 7, 2023 • edited Loading

DaoDaoNoCode left a comment

Choose a reason for hiding this comment

lucferbux left a comment

Choose a reason for hiding this comment

andrewballantyne Mar 8, 2023

Choose a reason for hiding this comment

jenny-s51 Mar 8, 2023 • edited Loading

Choose a reason for hiding this comment

andrewballantyne Mar 8, 2023

Choose a reason for hiding this comment

jenny-s51 Mar 9, 2023

Choose a reason for hiding this comment

andrewballantyne Mar 8, 2023

Choose a reason for hiding this comment

jenny-s51 Mar 8, 2023

Choose a reason for hiding this comment

andrewballantyne Mar 8, 2023

Choose a reason for hiding this comment

jenny-s51 Mar 9, 2023 • edited Loading

Choose a reason for hiding this comment

andrewballantyne commented Mar 8, 2023

andrewballantyne left a comment

Choose a reason for hiding this comment

openshift-ci bot commented Mar 9, 2023

openshift-ci bot commented Mar 9, 2023

jenny-s51 commented Mar 7, 2023 •

edited

Loading

Gkrumbach07 commented Mar 7, 2023 •

edited

Loading

jenny-s51 Mar 8, 2023 •

edited

Loading

jenny-s51 Mar 9, 2023 •

edited

Loading