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

feat(Textfield): ✨ New Textfield component #816

Merged
merged 17 commits into from
Sep 21, 2023
Merged

feat(Textfield): ✨ New Textfield component #816

merged 17 commits into from
Sep 21, 2023

Conversation

mimarz
Copy link
Collaborator

@mimarz mimarz commented Sep 14, 2023 •
edited
Loading

resolves #32
resolves #802
resolves #799

  • New Textfield component based on new design
  • As rudimentary and close to native input as possible
  • Dropped support for "formatting" as we have prefix/sufix for that now.
  • Currently we are ignoring the native input prop size.
    • How much is this used? Maybe provide a funnel prop (htmlSize, nativeSize or inputSize) for this or have size support string literal and number? 🤔

@mimarz mimarz changed the title feat: ✨ New Textfield component feat(Textfield): ✨ New Textfield component Sep 14, 2023
@github-actions
Copy link
Contributor

Storybook preview deployment is available at https://storybook-4lmtbcm7c-designsystemet.vercel.app

@github-actions
Copy link
Contributor

Storybook preview deployment is available at https://storybook-apkeept46-designsystemet.vercel.app

@mimarz mimarz marked this pull request as ready for review September 20, 2023 12:36
@mimarz mimarz added the 📔 storybook storybook.designsystemet.no label Sep 20, 2023
@mimarz mimarz marked this pull request as draft September 20, 2023 12:36
@mimarz mimarz marked this pull request as ready for review September 20, 2023 12:36
@github-actions
Copy link
Contributor

Storybook preview deployment is available at https://storybook-27obx3mxq-designsystemet.vercel.app

Magnusrm
Magnusrm reviewed Sep 20, 2023
View reviewed changes
Copy link
Contributor

@Magnusrm Magnusrm left a comment

Choose a reason for hiding this comment

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

Very neat! 💯 Just found a possible jsdoc oversight 🤓

packages/react/src/components/form/Textfield/useTextfield.ts Outdated Show resolved Hide resolved
@mimarz mimarz requested a review from Magnusrm September 20, 2023 13:05
Magnusrm
Magnusrm approved these changes Sep 20, 2023
View reviewed changes
Copy link
Contributor

@Magnusrm Magnusrm left a comment

Choose a reason for hiding this comment

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

LGTM! 🚀

@mrosvik
Copy link
Member

mrosvik commented Sep 20, 2023

⭐️ Nydelig!

@mimarz mimarz force-pushed the feat/new-textfield branch 2 times, most recently from 17806f6 to be983ed Compare September 21, 2023 06:02
TomasEng
TomasEng approved these changes Sep 21, 2023
View reviewed changes
Copy link
Contributor

@TomasEng TomasEng left a comment

Choose a reason for hiding this comment

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

Bra jobbet! Vi går altså bort fra InputWrapper også?

packages/react/src/components/form/CharacterCounter.tsx
/** The message indicating the remaining character limit. */
label?: (count: number) => string;
/** The description of the maximum character limit for screen readers. */
srLabel?: string;
Copy link
Contributor

Choose a reason for hiding this comment

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

Nå ser jeg at dette var her fra før, men jeg er usikker på "srLabel" er et beskrivende nok navn for dette. Skjermlesere er jo bare et eksempel på verktøy som benytter aria-describedby-attributtet. I praksis brukes det kanskje ikke til så mange andre formål (som jeg vet om i hvert fall), men i utgangspunktet er det jo der for å gjøre dom-en lesbar for alle typer verktøy. Hva med "description" eller noe i de baner?

@mimarz
Copy link
Collaborator Author

mimarz commented Sep 21, 2023

Bra jobbet! Vi går altså bort fra InputWrapper også?

Ja, jeg sjekket litt rundt på de diverse store og nasjonale designsystemene. Vi kommer nok til å bli tvunget til det uansett. Skal vi ha støtte for SSR og andre teknologier må vi nok finne på å ha eget css bibliotek med en fortståelige css klasser, da funker det dårlig med slike "skeleton" komponenter 🤔

Prøvde gjøre det så rett fram og enkelt som mulig i komponenten, så det ikke blir for uoversiktelig og ikke heller så gale om vi dupliserer oppsette i f.eks andre komponenter. Gjøre ting litt mer composable.

@mimarz mimarz merged commit 0c14cfa into main Sep 21, 2023
2 checks passed
@mimarz mimarz deleted the feat/new-textfield branch September 21, 2023 10:34
Magnusrm pushed a commit that referenced this pull request Sep 27, 2023
@mimarz @Magnusrm
feat(Textfield): ✨ New Textfield component (#816)
a032de0
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Magnusrm Magnusrm Magnusrm approved these changes

@TomasEng TomasEng TomasEng approved these changes

@Thunear Thunear Awaiting requested review from Thunear

@framitdavid framitdavid Awaiting requested review from framitdavid

Assignees
No one assigned
Labels
📔 storybook storybook.designsystemet.no
Projects
None yet
Milestone
No milestone
4 participants
@mimarz @mrosvik @TomasEng @Magnusrm