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.

Sign up for GitHub

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

Jump to bottom

Nye props på phonenumber #2087

Merged
merged 2 commits into from
Jun 18, 2024
Merged

Nye props på phonenumber #2087

merged 2 commits into from
Jun 18, 2024

Conversation

tuva-odegard
Copy link
Contributor

@tuva-odegard tuva-odegard commented Jun 14, 2024 •
edited
Loading

Oppdaterer PhoneNumber til å kunne ta inn numberInputProps og countryCodeInputProps sånn at det ikke blir så utrolig mange spesifikke props.
Gir også mulighet til å legge på required som en del av #1671

@tuva-odegard tuva-odegard requested a review from a team as a code owner June 14, 2024 08:49
@github-actions GitHub Actions
Copy link

Azure Static Web Apps: Your stage site is ready! Visit it here: https://black-beach-0d62d0d03-2087.westeurope.2.azurestaticapps.net

pethel
pethel previously approved these changes Jun 17, 2024
View reviewed changes
packages/ffe-form-react/src/PhoneNumber.tsx Outdated
@@ -11,6 +11,8 @@ export interface PhoneNumberProps {
onNumberChange?: React.ChangeEventHandler<HTMLInputElement>;
onCountryCodeBlur?: React.FocusEventHandler<HTMLInputElement>;
onNumberBlur?: React.FocusEventHandler<HTMLInputElement>;
countryCodeRequired?: boolean;
Copy link
Contributor

Choose a reason for hiding this comment

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

man kunde ju kanskje vurdert

numberInputProps: React.ComponentPropsWithoutRef<'input'>;
countryInputProps: React.ComponentPropsWithoutRef<'input'>;

Da man kan man slette en den av propsen her. Ville blitt en BREAKING CHANGE. Men dette er ju i linje med det andre i denne filen. Føljer dette er designsystemet fula lilla ankunge eftersom den ikke kan gå i en Inputgroup som alla annat

Copy link
Contributor

Choose a reason for hiding this comment

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

Gidder du ordne this should should i samme fila?
``

Copy link
Contributor Author

Choose a reason for hiding this comment

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

hmm det var jo lurt da, jeg tror jeg gjør det

@github-actions GitHub Actions
Copy link

Azure Static Web Apps: Your stage site is ready! Visit it here: https://black-beach-0d62d0d03-2087.westeurope.2.azurestaticapps.net

@antidecaf
Copy link
Contributor

Dette løser vel egentlig ikke issuet fullt ut sånn jeg leser det, i og med at det ikke er noen visuell merking av påkrevde felter? Kanskje kan required-propen(e) brukes til å toggle en klasse på label, f.eks .ffe-form-label--required, som legger til en asterisk etter teksten enten ved hjelp av ::after eller en span som skjules eller vises avhengig av verdi?

Jeg tenker også at to separate required-props er unødvendig, siden begge inputene implisitt vil være påkrevd hvis den ene er det.

@github-actions GitHub Actions
Copy link

Azure Static Web Apps: Your stage site is ready! Visit it here: https://black-beach-0d62d0d03-2087.westeurope.2.azurestaticapps.net

@tuva-odegard
Copy link
Contributor Author

Dette løser vel egentlig ikke issuet fullt ut sånn jeg leser det, i og med at det ikke er noen visuell merking av påkrevde felter? Kanskje kan required-propen(e) brukes til å toggle en klasse på label, f.eks .ffe-form-label--required, som legger til en asterisk etter teksten enten ved hjelp av ::after eller en span som skjules eller vises avhengig av verdi?

Jeg tenker også at to separate required-props er unødvendig, siden begge inputene implisitt vil være påkrevd hvis den ene er det.

@antidecaf
Yes, du nevnte det forrige uke!
Det kan vi godt gjøre teknisk, men usikker på om vi skaper mer kaos enn vi løser ved å gjøre det. Det er Andrea som har lagt det til, vet vi at det er et reelt behov? Alle har jo mulighet til å legge det til selv (label="Fornavn *" vs label="Fornavn"), så jeg tenker at det er best om det er opp til teamene. Hvis vi er usikre kan vi jo spørre i kanalen om det er ønskelig?

Har endret fra required spesifikt til inputProps som Peter foreslo, tipper det løser det siste du nevner 👍

@github-actions GitHub Actions
Copy link

Azure Static Web Apps: Your stage site is ready! Visit it here: https://black-beach-0d62d0d03-2087.westeurope.2.azurestaticapps.net

@github-actions GitHub Actions
Copy link

Azure Static Web Apps: Your stage site is ready! Visit it here: https://black-beach-0d62d0d03-2087.westeurope.2.azurestaticapps.net

1 similar comment
@github-actions GitHub Actions
Copy link

Azure Static Web Apps: Your stage site is ready! Visit it here: https://black-beach-0d62d0d03-2087.westeurope.2.azurestaticapps.net

@antidecaf
Copy link
Contributor

@antidecaf Yes, du nevnte det forrige uke! Det kan vi godt gjøre teknisk, men usikker på om vi skaper mer kaos enn vi løser ved å gjøre det. Det er Andrea som har lagt det til, vet vi at det er et reelt behov? Alle har jo mulighet til å legge det til selv (label="Fornavn *" vs label="Fornavn"), så jeg tenker at det er best om det er opp til teamene. Hvis vi er usikre kan vi jo spørre i kanalen om det er ønskelig?

Dette er et suksesskriterie i WCAG 2.1, og også en del av Forskrift om universell utforming av ikt. Med andre ord lovpålagt. Sånn sett litt rart vi ikke har hatt en løsning på det fra før.

image

@github-actions GitHub Actions
Copy link

Azure Static Web Apps: Your stage site is ready! Visit it here: https://black-beach-0d62d0d03-2087.westeurope.2.azurestaticapps.net

1 similar comment
@github-actions GitHub Actions
Copy link

Azure Static Web Apps: Your stage site is ready! Visit it here: https://black-beach-0d62d0d03-2087.westeurope.2.azurestaticapps.net

@tuva-odegard
Copy link
Contributor Author

ja lovpålagt, men trenger jo ikke være våres oppgave allikevel. Vi har tilrettelagt så teamene kan gjøre det! Men, jeg tror jeg legger ut et spørsmål i gruppa om at vi tenker og gjøre det og om det er noen objections i så fall

@antidecaf Yes, du nevnte det forrige uke! Det kan vi godt gjøre teknisk, men usikker på om vi skaper mer kaos enn vi løser ved å gjøre det. Det er Andrea som har lagt det til, vet vi at det er et reelt behov? Alle har jo mulighet til å legge det til selv (label="Fornavn *" vs label="Fornavn"), så jeg tenker at det er best om det er opp til teamene. Hvis vi er usikre kan vi jo spørre i kanalen om det er ønskelig?

Dette er et suksesskriterie i WCAG 2.1, og også en del av Forskrift om universell utforming av ikt. Med andre ord lovpålagt. Sånn sett litt rart vi ikke har hatt en løsning på det fra før.

image

@antidecaf
Copy link
Contributor

ja lovpålagt, men trenger jo ikke være våres oppgave allikevel. Vi har tilrettelagt så teamene kan gjøre det! Men, jeg tror jeg legger ut et spørsmål i gruppa om at vi tenker og gjøre det og om det er noen objections i så fall

Jeg er ikke helt enig i at vi har tilrettelagt nok hvis vi ikke har en standardisert måte å markere obligatoriske felter på i skjemakomponentene. Å overlate denne type ting til konsumentene er sånt som erfaringsmessig fører til at det blir løst på forskjellige måter i forskjellige apper.

@github-actions GitHub Actions
Copy link

Azure Static Web Apps: Your stage site is ready! Visit it here: https://black-beach-0d62d0d03-2087.westeurope.2.azurestaticapps.net

1 similar comment
@github-actions GitHub Actions
Copy link

Azure Static Web Apps: Your stage site is ready! Visit it here: https://black-beach-0d62d0d03-2087.westeurope.2.azurestaticapps.net

@tuva-odegard tuva-odegard changed the title Required på phonenumber Nye props på phonenumber Jun 18, 2024
@tuva-odegard tuva-odegard merged commit 2b43a5d into develop Jun 18, 2024
3 checks passed
@tuva-odegard tuva-odegard deleted the required-labels branch June 18, 2024 13:53
@tuva-odegard tuva-odegard self-assigned this Jun 18, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@pethel pethel pethel approved these changes

@antidecaf antidecaf antidecaf approved these changes

Assignees

@tuva-odegard tuva-odegard

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@tuva-odegard @antidecaf @pethel