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

feat(Forms): add inline HelpButton to all Field.* components as default (with option to open in Dialog) #4282

Merged
merged 7 commits into from
Nov 17, 2024

Conversation

tujoworker
Copy link
Member

@tujoworker tujoworker commented Nov 14, 2024

This PR changes the existing behavior of the help prop for Fields that previously supported it. The content now opens inline instead of in a Dialog.

The help prop has now all of these optional props:

<Field.String
  label="Label text"
  help={{
    title: 'Help is available',
    content:
      'Take the time to help other people without expecting a reward or gratitude is definitely important in living an optimistic life.',
    open: false,// only for the inline variant
    renderAs: 'dialog'
  }}
/>

We use this prop description:

Provide help content for the field using title and content as a string or React.Node. Additionally, you can set open to true to display the inline help or use renderAs set to dialog to render the content in a Dialog (recommended for larger amounts of content).

Examples with inline "HelpButton":

NB: We do not document the internals of the inline version at this time. This can be included in another PR. The reason is that it would require additional tests, examples, and documentation. It is also an "isolated" feature specific to the HelpButton. However, as of this writing, I am unsure if we will ever document it, as integration is not straightforward. There are several considerations to address when implementing it.

Copy link

vercel bot commented Nov 14, 2024

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 Nov 17, 2024 8:47pm

Copy link

codesandbox-ci bot commented Nov 14, 2024

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.

@langz

This comment was marked as resolved.

@langz

This comment was marked as resolved.

@tujoworker tujoworker requested a review from langz November 15, 2024 08:08
@langz
Copy link
Contributor

langz commented Nov 15, 2024

I tested tabbing with keyboard, and it works great 👍
Found one edge-case when using enter/space to opening the help text, in the following example: https://eufemia-git-feat-forms-inline-help-button-eufemia.vercel.app/uilib/extensions/forms/create-component/FieldBlock/demos/#inline-help-button-vertical-label-description

It only happens when spacing/entering, not when clicking.
Sometimes it can be hard to reproduce, but double spacing/entering twice fast can lead to it, but is not necessary(in the video provided, I don't do it fast).

The issue/error is that it somehow focuses the section(?)-element inside the help text, which I don't think it should? As it doesn't do so when clicking with the mouse.

Screen.Recording.2024-11-15.at.09.21.06.mov


<Examples.InlineHelpButtonVerticalLabelDescription />

### Inline help button (horizontal label)
Copy link
Contributor

@langz langz Nov 15, 2024

Choose a reason for hiding this comment

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

Suggested change
### Inline help button (horizontal label)
### Inline help button (horizontal label)
Help text will appear above the field when using horizontal labels.

Makes sense, but just got surprised when I tested this https://eufemia-git-feat-forms-inline-help-button-eufemia.vercel.app/uilib/extensions/forms/create-component/FieldBlock/demos/#inline-help-button-horizontal-label, as I sort of still expected it to show up the same as with vertical labels, but that's not possible 😅

Copy link
Member Author

Choose a reason for hiding this comment

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

I gave it a try. And I think its better to have it appear always "below" the label. Also when the layout is horizontal.

@langz
Copy link
Contributor

langz commented Nov 15, 2024

I tested the following example with the following code:

<Field.Composition info="Info at the bottom" width="large">
  <Field.String label="Field A with a long label" width="medium" help={{
    title: 'Help title',
    content: 'Help content',
  }}/>
  <Field.String label="Field B" width="stretch" help={{
    title: 'Help title',
    content: 'Help content',
  }} />
</Field.Composition>

And for the field with width="stretch" the help text appeared(animated in) in the opposite way then usual:

Screen.Recording.2024-11-15.at.10.10.12.mov

@langz
Copy link
Contributor

langz commented Nov 15, 2024

I made a CSB to try out help together with layout="horizontal".
Not sure what we should support or not for Field.Composition
Here's some observations:

Upload: help goes over border
Screenshot 2024-11-15 at 11 42 17

Field.Selection input fields changes position and/or width when opening/closing the help:

Screen.Recording.2024-11-15.at.11.44.04.mov
Screen.Recording.2024-11-15.at.11.44.55.mov
Screen.Recording.2024-11-15.at.11.44.34.mov
Screen.Recording.2024-11-15.at.11.44.44.mov

Further, it's also not all Fields that support layout="horizontal", but that's probably on purpose, and then we don't have to care about the "styling issues" if it's not supported for these components/fields:

Screenshot 2024-11-15 at 11 20 39

@langz
Copy link
Contributor

langz commented Nov 15, 2024

Should we add this help functionality to the ValueBlock as well?
Could see us doing so at a point, and perhaps even add inheritHelp

@langz
Copy link
Contributor

langz commented Nov 15, 2024

Field. BankAccountNumber when layout horizontal, changes position and/or width when opening/closing the help:

The same happens with almost all feature fields

Screen.Recording.2024-11-15.at.12.02.41.mov

@langz
Copy link
Contributor

langz commented Nov 15, 2024

Here's a CSB displaying some possible alignment issues when using help with layout vertical for fields that have two fields., in a card
https://codesandbox.io/p/sandbox/funny-ishizaka-sgcp6x?workspaceId=9eb653cb-4406-4b8f-bc96-714927cc7fcd

image

It's like it takes a bit more width than the width of the input field.

@tujoworker
Copy link
Member Author

I think I have covered now all of your fantastic feedback.

The rotation animation runs now also when hiding the help content.

Some fields do not support a horizontal layout, such as PostcalCode and PhoneNumber. But also Composition does not support it.

@tujoworker
Copy link
Member Author

Thanks, sure, fine that we don't support horizontal layout in PostCodeAndCity, PhoneNumber, Composition, etc.
But how could/should we say that to our users?
In the docs, it seems like we should support it, at least for:

I have update the docs for the PostalCodeAndCity field. PhoneNumber is already taken care of.

I'm still able to reproduce this, but I'm not sure how important this is to fix, or if actually is anything to fix 🤔

I'm not able to reproduce it anymore. Also, we already have dnb-no-focus to the section element. Not sure what I do wrong 😄

I'm still able to reproduce this as well, https://codesandbox.io/p/sandbox/autumn-glade-r4n5v6?workspaceId=9eb653cb-4406-4b8f-bc96-714927cc7fcd

Found another solution.

Here's a CSB displaying some possible alignment issues when using help with layout vertical for fields that have two fields., in a card https://codesandbox.io/p/sandbox/funny-ishizaka-sgcp6x?workspaceId=9eb653cb-4406-4b8f-bc96-714927cc7fcd

Also enhanced + added a visual test.

@langz
Copy link
Contributor

langz commented Nov 16, 2024

Thanks for the updates @tujoworker
I've forked and updated the Eufemia version to be of latest commit f63c8181 in this PR for the following CSBs:

I've not tested them yet.

EDIT: I've tested them now, and it looks better 👏
Only two thoughts left from me:

@langz
Copy link
Contributor

langz commented Nov 16, 2024

I quickly re-tested tabbing and openeing/closing with enter and space keys, and I feel it works worse now.
I'm not able to reproduce the active/focused state of the section(?), which is good, but when using space and enter now it works worse.

Using space, first time it opens, next time I do space it scrolls down the page(even though the button looks active/focused).

Using enter, first time it opens, next time I press enter it won't close (even though the button looks active/focused):

Screen.Recording.2024-11-16.at.12.46.25.mov

https://eufemia-git-feat-forms-inline-help-button-eufemia.vercel.app/uilib/extensions/forms/create-component/FieldBlock/demos/#inline-help-button-vertical-only

Maybe we just revert this funcionality, and fix it if it becomes a issue down the line, I'm not sure if the issue initially reported is critical or not.

@langz
Copy link
Contributor

langz commented Nov 16, 2024

Since last time I checked the CSBs, the All fields horizontal latest commit looks quite different from what it did in the previous commit/change.
Is it intentional that it doesn't use the full width anymore when layout is horizontal?
For me it's fine, just curious 👍

How it was:

image

How it is now:

image

@langz
Copy link
Contributor

langz commented Nov 16, 2024

Some fields do not support a horizontal layout, such as PostcalCode and PhoneNumber. But also Composition does not support it.

Do we have to support it in the future?
Does it today exist a component that you can wrap all fields in, and say layout=horizontal, like in the form.handler Field.Provider, etc? Probably Field.Provider. Wouldn't the form look very strange now, when some Fields support horizontal and other don't? Or perhaps those fields(PostalCode and PhoneNumber) looks best as vertical, even when in a form where all other fields are horizontal? 🤔

Here's a CSB using Field.Provider with layout horizontal.

Sidenote: Not sure why, but in that CSB I'm not able to enter anything in the postalcode field 🤔

@tujoworker tujoworker force-pushed the feat/forms-inline-help-button branch 2 times, most recently from 1e5c72d to 24ab19b Compare November 17, 2024 08:14
@tujoworker
Copy link
Member Author

Using space, first time it opens, next time I do space it scrolls down the page(even though the button looks active/focused).
Using enter, first time it opens, next time I press enter it won't close (even though the button looks active/focused):

We set focus on the section so screen readers can navigate in there and also get read out the text. Even if the content is placed on a different DOM order.
But also so we can show the "hover" state (larger than focus).
I added support for the enter/space key to close it again. From before we had support for esc key.

Is it intentional that it doesn't use the full width anymore when layout is horizontal?

Yes. Not it is aligned on how the status messages looks. Same border radius too. I think this is the most logical way to display this information on in a consistent way.

Do we have to support it in the future?

I have never seen any PhoneNumber etc. layout that could work with these fields. So as of now, this is how it is. Most horizontal layouts are used within a small part of the form. So we do not need to support necessarily all fields with that layout.

@langz
Copy link
Contributor

langz commented Nov 17, 2024

Using space, first time it opens, next time I do space it scrolls down the page(even though the button looks active/focused).

Using enter, first time it opens, next time I press enter it won't close (even though the button looks active/focused):

We set focus on the section so screen readers can navigate in there and also get read out the text. Even if the content is placed on a different DOM order.

But also so we can show the "hover" state (larger than focus).

I added support for the enter/space key to close it again. From before we had support for esc key.

Is it intentional that it doesn't use the full width anymore when layout is horizontal?

Yes. Not it is aligned on how the status messages looks. Same border radius too. I think this is the most logical way to display this information on in a consistent way.

Do we have to support it in the future?

I have never seen any PhoneNumber etc. layout that could work with these fields. So as of now, this is how it is. Most horizontal layouts are used within a small part of the form. So we do not need to support necessarily all fields with that layout.

Thanks Tobias, that sounds smart.
I'm very pleased with this.
Will take a quick round of testing now

@langz
Copy link
Contributor

langz commented Nov 17, 2024

Thanks again the updates @tujoworker 🚀
I've forked and updated the Eufemia version to be of latest commit 24ab19b3 in this PR for the following CSBs:

Copy link
Contributor

@langz langz left a comment

Choose a reason for hiding this comment

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

Thanks for the great job 👏
Works perfect now :)

@tujoworker tujoworker force-pushed the feat/forms-inline-help-button branch from 24ab19b to fb9b030 Compare November 17, 2024 20:38
@tujoworker tujoworker force-pushed the feat/forms-inline-help-button branch from fb9b030 to c421c4e Compare November 17, 2024 20:38
@langz langz merged commit e869a60 into main Nov 17, 2024
10 checks passed
@langz langz deleted the feat/forms-inline-help-button branch November 17, 2024 22:29
tujoworker pushed a commit that referenced this pull request Nov 18, 2024
## [10.56.0](v10.55.1...v10.56.0) (2024-11-18)

### 🐛 Bug Fixes

* **DatePicker:** ensure that `endMonth` does not fallback to `startMonth` when `endMonth` prop is defined ([#4254](#4254)) ([5281257](5281257))
* **Forms:** render given elements to `warning` and `info` properties ([#4261](#4261)) ([d60de25](d60de25))

### ⚡ Refactoring

* **DatePicker:** convert properties to camel case and deprecate those with snake case ([#4273](#4273)) ([a69a8aa](a69a8aa))

### ✨ Features

* **Field.Date:** add DatePickerProps ([#4160](#4160)) ([6a3765b](6a3765b))
* **Forms:** add `asyncFileHandler` to Field.Upload to support async file handling during upload ([#4281](#4281)) ([030a09c](030a09c))
* **Forms:** add inline HelpButton to all Field.* components as default (with option to open in Dialog) ([#4282](#4282)) ([e869a60](e869a60))
* **Forms:** add support for multiple info, warning and error messages given by an array ([#4284](#4284)) ([78f2fe8](78f2fe8))
* **ListFormat:** adds spacing properties ([#4255](#4255)) ([c72d999](c72d999))
* **Paragraph:** handle nested paragraphs (to be `span`'s) ([#4251](#4251)) ([ca3bbde](ca3bbde))
@tujoworker
Copy link
Member Author

🎉 This PR is included in version 10.56.0 🎉

The release is available on:

Your semantic-release bot 📦🚀

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

Successfully merging this pull request may close these issues.

2 participants