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

Make Spinner accessible #2416

Open
wants to merge 3 commits into
base: master
Choose a base branch
from
Open

Make Spinner accessible #2416

wants to merge 3 commits into from

Conversation

rauboti
Copy link
Contributor

@rauboti rauboti commented Aug 13, 2024

Forsøk på bedring av Spinner-komponenten, i tråd med https://github.com/NDLANO/Issues/issues/4127

@rauboti rauboti requested a review from a team August 13, 2024 09:56
@rauboti rauboti marked this pull request as ready for review August 13, 2024 09:56
<StyledSpinner
aria-busy="true"
aria-live="polite"
aria-valuetext="Loading"
Copy link
Member

Choose a reason for hiding this comment

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

Bør vel puttes inn i oversettelser.

Copy link
Contributor

@Jonas-C Jonas-C Aug 13, 2024

Choose a reason for hiding this comment

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

Nei. Vi støtter ikke oversettelser i primitives, og det synes jeg ikke vi skal heller. Dette burde generelt sett settes der den brukes, for å gi mer nøyaktige oversettelser.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Må innrømme jeg hadde over gjennomsnittet lyst te å legge inn oversettelser her, men sida denne valuetexten KAN være noe forskjellig avhengig av hvor spinneren brukes (trenger ikke være, men kan), ble det hakket mindre viktig.
Denne fungerer derfor kun som en fallbackverdi. Jo flere ting man må huske å sette på komponentene, jo lettere er det for at noe går i glømmeboka. Spesielt attributter som brukes mindre jevnlig er utsatt for å kunne glemmes. For meg er det argument godt nok til å beholde den her, som en fallback. Men den bør definitivt overskrives der komponenten brukes.

Copy link
Contributor

Choose a reason for hiding this comment

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

Denne må vel overstyres alle steder, da hovedspråket på siden er norsk. Da er det like greit å gjøre den required v.h.a typescript.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Da liker jeg faktisk oversettelser bedre, ev. under egen gruppering composites eller noe, dersom primitives skal være fri for slikt. Jeg mener det ikke er sannsynlig at den tekstverdien kommer til å ende opp med å være forskjellig på alle steder, og da mener jeg det er tungvint å skulle legge den inn hvert sted komponenten brukes hvis det kan unngås (og heller la den være overstyrbar de steder den må overstyres).
Jeg tar den ut og legger den som required for typescript nå (så kan det heller gjøres om senere), men ber om at vi gjør en vurdering av om vi kan håndtere slikt bedre.

@rauboti rauboti force-pushed the accessible-spinner branch from 141e453 to 6dbf49f Compare August 14, 2024 06:26
<StyledSpinner
aria-busy="true"
aria-live="polite"
role="progressbar"
Copy link
Contributor

Choose a reason for hiding this comment

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

Dette skal nok ikke være en progressbar, men heller en status. Vi har ikke noe progress på spinneren vår. Enten så eksisterer den eller så eksisterer den ikke. progressbar skal brukes til faktisk målbare distanser.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Det er ikke helt presist. Progressbar kan fint brukes for ikke-definerte måledistanser, selv om det er mindre vanlig. Lasting er en status, men det er også en progresjon selv om vi ikke måler og viser den. Her er det gjort med vilje for å kunne støtte aria-valuetext. Men sida jeg også har valgt polite over assertive, så antar jeg at jeg kan bruke aria-label til samme info, og bare flatt sette aria-status.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Forøvrig en sammenligning mellom role="status" (første bilde) og role="progressbar" (andre bildet) når jeg blar igjennom ei side som fortsatt har en spinner:
image
Og nr 2:
image

I den første, som har role="status", dukker kun teksten opp fordi jeg har lagt den som en paragraph; aria-label plukka den ikke opp. Nøkkelinfo er hhv nest siste og siste linje, teksten "Venter i spenning". Jeg prøvde også å bytte ut Spinner-komponenten med en vanlig div, i tilfelle det var noe mer ark (mer for å utelukke)
image

aria-hidden fordi jeg ikke vil lese opp bokstaven T. Og den hopper da glatt over det når jeg blar igjennom sida, og oppfatter ingenting. Er åpen for at jeg kanskje gjør noe feil, men det skal litt til å ødelegge speech vieweren, og jeg kan ikke se role="status" klarer å gi en skjermleser det samme som role="progressbar" her.

@Jonas-C
Copy link
Contributor

Jonas-C commented Aug 14, 2024

Tenker vi heller kan bevege oss mot det gov.uk holder på med
https://design.homeoffice.gov.uk/components?name=Loading%20spinner

@rauboti rauboti force-pushed the accessible-spinner branch from 36c7bae to 2db770f Compare August 28, 2024 06:03
@gunnarvelle
Copy link
Member

Er dette noko vi kan ta videre?

@Jonas-C
Copy link
Contributor

Jonas-C commented Sep 6, 2024

Har lyst til å få sett litt mer ordentlig på denne når ting roer seg litt ned igjen. Mener å huske at jeg og Gaute ikke var helt enige mtp hvordan ting skulle oppføre seg!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants