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

chore(forms): remove labelDescription and labelSecondary #3209

Merged
merged 1 commit into from
Jan 16, 2024
Merged

chore(forms): remove labelDescription and labelSecondary #3209

merged 1 commit into from
Jan 16, 2024

Conversation

tujoworker
Copy link
Member

@tujoworker tujoworker commented Jan 10, 2024 •
edited
Loading

This PR did have a failure, where we removed the characterCounter – it got added back in with this PR #3210 – but it did not get released with the same version, which was a bummer!

@vercel Vercel
Copy link

vercel bot commented Jan 10, 2024 •
edited
Loading

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Comments Updated (UTC)
eufemia ✅ Ready (Inspect) Visit Preview 💬 Add feedback Jan 16, 2024 2:13pm

@codesandbox-ci CodeSandbox CI
Copy link

codesandbox-ci bot commented Jan 10, 2024 •
edited
Loading

This pull request is automatically built and testable in CodeSandbox.

To see build info of the built libraries, click here or the icon next to each commit SHA.

Latest deployment of this branch, based on commit 2692944:

Sandbox Source
eufemia-starter Configuration

@tujoworker tujoworker changed the title chore(forms): remove labelDescription, labelSecondary and characterCounter chore(forms): remove labelDescription, labelSecondary Jan 10, 2024
@tujoworker tujoworker changed the title chore(forms): remove labelDescription, labelSecondary chore(forms): remove labelDescription and labelSecondary Jan 10, 2024
@tujoworker tujoworker requested a review from joakbjerk January 11, 2024 10:30
joakbjerk
joakbjerk approved these changes Jan 11, 2024
View reviewed changes
Copy link
Contributor

@joakbjerk joakbjerk left a comment

Choose a reason for hiding this comment

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

🙏

@tujoworker tujoworker force-pushed the chore/string-removelabel-and-character-counter branch from d15c9ff to f587686 Compare January 16, 2024 07:57
@tujoworker tujoworker force-pushed the chore/string-removelabel-and-character-counter branch 2 times, most recently from de3f00d to 84f2fc1 Compare January 16, 2024 12:52
@tujoworker tujoworker force-pushed the chore/string-removelabel-and-character-counter branch from 84f2fc1 to 2692944 Compare January 16, 2024 14:07
@tujoworker tujoworker merged commit 92ad8e8 into main Jan 16, 2024
10 checks passed
@tujoworker tujoworker deleted the chore/string-removelabel-and-character-counter branch January 16, 2024 14:36
@tujoworker
Copy link
Member Author

🎉 This PR is included in version 10.18.0 🎉

The release is available on:

Your semantic-release bot 📦🚀

@kimroen
Copy link
Contributor

kimroen commented Jan 19, 2024 •
edited
Loading

@tujoworker This PR also removes the characterCounter prop from Field.String. I cannot quite tell if this was on purpose or not, but since the MR clarifies the documentation for characterCounter and the changelog doesn't mention its removal I would have to think that it was unintentional.

We were using this property, so we'll hold off on upgrading to 10.18 for now, but please let me know if this was done intentionally, as we would then have to look at alternatives.

@tujoworker
Copy link
Member Author

@kimroen Thank you for following up this potential issue.

We have already added a better version to the original textarea, this time with screen reader support.

PR: #3210

But you use it on single line inputs?

@kimroen
Copy link
Contributor

kimroen commented Jan 19, 2024

Thanks! It sounds like the fields documentation might need updating then, and mentioning it in the release notes would be appreciated 👏

We do indeed, for short fields like KID-number for instance.

@kimroen
Copy link
Contributor

kimroen commented Jan 19, 2024

It sounds like the fields documentation might need updating then

Nope, this was just me misreading the diff - I see now you're updating the documentation for the property for the textarea component!

@tujoworker
Copy link
Member Author

Yeah, regarding the release notes – I strongly agree and I'm sorry for the confusion. The labelDescription and labelSecondary was never a part of Eufeia. But projects used it anyways. That's not how it should be. But I think, at least the labelDescription we should get back in.

Regarding the counter; UX decided, they don't want a counter for normal fields. Also, I just got feedback, that it should up and down. But let me see what we can do about the counter for inputs.

@tujoworker tujoworker mentioned this pull request Jan 21, 2024
Merged
@tujoworker
Copy link
Member Author

We add back in labelDescription for v10.19 🙌

@kimroen
Copy link
Contributor

kimroen commented Jan 22, 2024

Thanks for following up here!

Regarding the counter; UX decided, they don't want a counter for normal fields.

Thanks, I'll take this back to our UX designers to see what they think we should do instead for these fields.

Also, I just got feedback, that it should up and down.

I don't think I understand what this means 🙈

tujoworker added a commit that referenced this pull request Jan 22, 2024
@tujoworker
feat(forms): add labelDescription prop to fields (labelSecondary go…
00c278c 
…t removed) (#3251)

`labelDescription` was a part of forms before, but where removed in this
PR #3209.

But it turns out, UX wants this for form fields. This time it gets
officially included. We need to consider to add this to FormLabel at a
later stage. But that involves a lot more work.

Also, `Exipry` needed a larger rewrite in order to make
`labelDescription` work. This change will be included in this PR #3252
tujoworker pushed a commit that referenced this pull request Jan 22, 2024
@semantic-release-bot
chore(release): 10.19.0 [skip ci]
3ace85c 
## [10.19.0](v10.18.0...v10.19.0) (2024-01-22)

### 🐛 Bug Fixes

* **Autocomplete:** replace existing aria-live handling with the AriaLive component  ([#3258](#3258)) ([0ec06ca](0ec06ca))
* **Breadcrumb:** fix rehydration disturbance ([#3254](#3254)) ([dcf3a8b](dcf3a8b)), closes [#2762](#2762) [#2671](#2671)
* **DrawerList:** update original data when data prop changes ([#3247](#3247)) ([d1b03c2](d1b03c2))
* **Field:** show error state without error object if parent FieldBlock has error ([#3225](#3225)) ([35fe238](35fe238)), closes [#2958](#2958)
* **Flex.Container:** ensure rowGap=false has effect ([#3242](#3242)) ([e18ddfd](e18ddfd))
* **forms:** rename `contents` to `content` ([#3257](#3257)) ([2c9a397](2c9a397))
* **Input:** should not clear input value with escape key ([#3235](#3235)) ([979b3e3](979b3e3))
* **PhoneNumber:** handle pattern, schema and validator with country code ([#3249](#3249)) ([ed115d5](ed115d5))
* **Table.Accordion:** prevent accordion from opening on label click ([#3228](#3228)) ([ee5014f](ee5014f))
* **Textarea:** correct outline to be inset ([#3237](#3237)) ([6433470](6433470))

### ✨ Features

* **AriaLive:** add new component ([#3217](#3217)) ([7c79a54](7c79a54))
* **Flex:** add `line-framed` to divider ([#3244](#3244)) ([1aa3338](1aa3338)), closes [#3242](#3242) [#3245](#3245)
* **forms:** add labelDescription prop to fields (`labelSecondary` got removed) ([#3251](#3251)) ([00c278c](00c278c)), closes [#3209](#3209) [#3252](#3252)
* **NumberUtils.format:** Only return object if returnAria: true ([#3262](#3262)) ([ca4315f](ca4315f))
* **Section:** add support for backgroundColor=transparent ([#3255](#3255)) ([07e1545](07e1545))
* **Textarea:** add characterCounter ([#3210](#3210)) ([5c9dde9](5c9dde9)), closes [#3217](#3217)
* **TextCounter:** add new fragment used in Textarea ([#3250](#3250)) ([3093c28](3093c28)), closes [#3210](#3210)
@tujoworker
Copy link
Member Author

@kimroen we have the same props as in v10.17, except labelSecondary. While characterCounter accepts now a number (max) or the props for TextCounter.

@kimroen
Copy link
Contributor

kimroen commented Jan 25, 2024

Thanks! I tried this now, and it seems that characterCounter still has no effect on single-line fields (but it's a valid prop again now at least). Is that correct and intentional? If so, it seems I misunderstood what you were saying here.

I took this back to UX, and they had been part of discussions about this and did not have the impression that the counter would only be for multiline input fields.

Separately, the docs for Fields.String still says that characterCounter accepts a boolean, but the types seem to suggest that it only accepts a number or an object.

@kimroen
Copy link
Contributor

kimroen commented Jan 26, 2024

Separately, the docs for Fields.String still says that characterCounter accepts a boolean, but the types seem to suggest that it only accepts a number or an object.

Looks like the type is correct, but perhaps supporting boolean and falling back to the supplied maxLength would be a little more backwards compatible if it used to support boolean too?

I like this new counter for multiline fields though, don't get me wrong! We'll go back to the drawing board here for how to design the KID number field, and just need to keep this in mind when upgrading Eufemia for existing applications.

@tujoworker
Copy link
Member Author

Are you able to use the TextCounter for your use case?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@joakbjerk joakbjerk joakbjerk approved these changes

Assignees
No one assigned
Labels
released
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@tujoworker @kimroen @joakbjerk