-
Notifications
You must be signed in to change notification settings - Fork 31
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: disabled/readOnly styling for TextInput #4609
Conversation
✅ Forhåndsvisning: https://jokul.fremtind.no/preview/olejorgenbakken/issue4566/ Forhåndsvisningen blir tilgjengelig innen et par minutter. Den fjernes automatisk når pull requesten lukkes. |
0241bff
to
ff88318
Compare
Nå er det også lagt til mulighet for å gi en grunn til at det et input felt ikke er tilgjengelig. Dette gjøres med Merk at dette ikke vil være tilgjengelig for skjermlesere, med en tanke om at det blir veldig støyete. Her er jeg veldig åpen for at vi gjør det tilgjengelig for skjermlesere, så gjerne kom med deres meninger om hvordan det kan løses på en god måte 😸 |
e63e157
to
db005a4
Compare
packages/jokul/src/components/text-input/styles/text-input.scss
Outdated
Show resolved
Hide resolved
inline?: boolean; | ||
inputClassName?: string; | ||
} | ||
export type TextInputProps = Omit<InputGroupProps, "children"> & |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
En interessant diskusjon er sikkert om vi burde håndtere det som en breaking change når vi endrer fra et interfjes til en type. Dersom noen ute i teamene har valgt å extende TextInputProps
av en eller annen grunn så må de også endre ting hos seg.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Denne er jeg litt usikker på om jeg får gjort så mye med. Tenker du det er mer fornuftig å beholde den som et interface, eller skal vi spørre på forum?
packages/jokul/src/shared/input/styles/shared-input-styles.scss
Outdated
Show resolved
Hide resolved
Ser nice ut for meg, usikker på om det burde presenteres på forum før det slippes? |
Ser bra ut. Kunne vært greit å se disse statesene på et par andre komponenter også for at sjekke at konseptet funker :) |
Jeg ser at GitHub Primer har valgt å ikke støtte Gjør man dette har skjermleserbrukere og de som navigerer med tastatur også mulighet til å få med seg at feltet faktisk eksisterer, som vel må være viktig dersom man velger disabled fremfor skjult felt. Den største utfordringen med å gjøre det på denne måten er at vi må lage egen logikk for å unngå uønsket interaksjon med feltet (tror jeg). Red.: Her er et utdrag av bakgrunnen deres for hvordan de har implementert det de kaller inactive button, som jeg mener er relevant også for våre input-felter (min utheving):
|
Dette var sånn jeg hadde tenkt å løse det faktisk, av akkurat samme årsaker haha. Eneste spørsmålet da er om vi skal fjerne |
db005a4
to
1a4158d
Compare
1a4158d
to
39a5789
Compare
Den kan allikevel være nyttig på interne flater, og er derfor mulig fortsatt. |
df24046
to
3bf376f
Compare
ISSUES CLOSED: #4605
3bf376f
to
85b1077
Compare
Lukker denne PR-en til fordel for å lage en ny med kun styling i førsteomgang |
ISSUES CLOSED: #4605
Legger til styling for disabled og read-only states i TextInput komponenten.
🎯 Sjekkliste
pnpm build
ogpnpm ci:test
gir ingen feil