dnbexperience / eufemia

DNB Design System
https://eufemia.dnb.no
Other
53 stars 32 forks source link

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

Closed tujoworker closed 4 days ago

tujoworker commented 1 week ago

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.

vercel[bot] commented 1 week ago

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
codesandbox-ci[bot] commented 1 week ago

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 commented 1 week ago

When clicking the question mark and opening, the question mark changes to an x which animates. Then when clicking the x and closing, it just changes to the question mark. Should it do the reverse of when opening, to animate the x back, before displaying question mark?

This is just thoughts, not very important to me, and I'm not 100% sure what's the correct behaviour here.

https://github.com/user-attachments/assets/62c6363f-1925-4960-905d-cfe43c478974

langz commented 1 week ago

In the following examples

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

The animation feels a bit different than in https://eufemia-git-feat-forms-inline-help-button-eufemia.vercel.app/uilib/extensions/forms/create-component/FieldBlock/demos/#inline-help-button-vertical-only, it doesn't keep the spacing below the inline help text:

https://github.com/user-attachments/assets/266deeca-cde7-417c-8037-26c75df76356

Image of animation at a point. Here we can see the top border of the input field is gone/covered: image

Because of this, it feels like it changes/does something with the top border of the input field(of course it does not, but visually it feels so, that it flashes):

https://github.com/user-attachments/assets/9fd1eba7-523f-4323-b013-6e614910eac7

langz commented 1 week ago

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.

https://github.com/user-attachments/assets/5d36b952-a72b-4107-9ad3-b1f2e1227a55

langz commented 1 week ago

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:

https://github.com/user-attachments/assets/24bbe9d7-295b-400e-a021-4b8c9c8bef6a

langz commented 1 week ago

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:

https://github.com/user-attachments/assets/070baa66-a5fd-4ef7-8bf1-4dae97d59480

https://github.com/user-attachments/assets/11523561-540e-4d74-91cf-c0711c739224

https://github.com/user-attachments/assets/73564bc5-bf20-4dd3-9b5e-c8df253d7e74

https://github.com/user-attachments/assets/6e04cb05-b088-40d7-910e-38adb56050eb

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 commented 1 week ago

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 commented 1 week ago

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

The same happens with almost all feature fields

https://github.com/user-attachments/assets/7c815c5a-e264-4fe6-bc62-ac6af1583a87

langz commented 1 week ago

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 commented 6 days ago

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.

langz commented 6 days ago

I've forked and updated the eufemia version to be of latest commit in this PR for the following CSBs:

langz commented 6 days ago

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.

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:

langz commented 6 days ago

When clicking the question mark and opening, the question mark changes to an x which animates. Then when clicking the x and closing, it just changes to the question mark. Should it do the reverse of when opening, to animate the x back, before displaying question mark?

This is just thoughts, not very important to me, and I'm not 100% sure what's the correct behaviour here.

trim.BBA79D28-B838-4F3B-8551-29D1D571C1F9.MOV

This looks great now โค๏ธ

langz commented 6 days ago

In the following examples

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

The animation feels a bit different than in https://eufemia-git-feat-forms-inline-help-button-eufemia.vercel.app/uilib/extensions/forms/create-component/FieldBlock/demos/#inline-help-button-vertical-only, it doesn't keep the spacing below the inline help text:

trim.6603225A-06AE-48FC-809C-19FB8AA2ABDD.MOV Image of animation at a point. Here we can see the top border of the input field is gone/covered: image

Because of this, it feels like it changes/does something with the top border of the input field(of course it does not, but visually it feels so, that it flashes):

trim.2067C2DA-FF8C-4143-A5D5-F2A8815981B5.MOV

This looks great now ๐Ÿš€

langz commented 6 days ago

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

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 ๐Ÿค”

langz commented 6 days ago

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

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

Can also be seen when opening/closing the help buttons for:

in the following CSB https://codesandbox.io/p/sandbox/elastic-tom-zxhk2r?workspaceId=9eb653cb-4406-4b8f-bc96-714927cc7fcd

langz commented 6 days ago

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.

I was able to reproduce this as well, https://codesandbox.io/p/sandbox/nifty-booth-rvkvt2?workspaceId=9eb653cb-4406-4b8f-bc96-714927cc7fcd

tujoworker commented 6 days ago

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 commented 6 days ago

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 commented 6 days ago

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):

https://github.com/user-attachments/assets/c72715cf-e24f-455a-821f-f5a435ed4574

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 commented 6 days ago

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 commented 6 days ago

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 commented 5 days ago

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 commented 5 days ago

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 commented 5 days ago

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:

tujoworker commented 4 days ago

:tada: This PR is included in version 10.56.0 :tada:

The release is available on:

Your semantic-release bot :package::rocket: