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 showConfirmDialog to Iterate.RemoveButton #4330

Merged
merged 6 commits into from
Nov 25, 2024

Conversation

tujoworker
Copy link
Member

@tujoworker tujoworker commented Nov 22, 2024

Based on #4329

Here is a PR Preview.

Copy link

vercel bot commented Nov 22, 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 25, 2024 0:02am

@tujoworker tujoworker marked this pull request as ready for review November 22, 2024 19:53
Copy link

codesandbox-ci bot commented Nov 22, 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
Copy link
Contributor

langz commented Nov 22, 2024

Pr description says based on #4202, that can't be correct or?🤔

EDIT: #4329 is the correct one 🙏

@langz
Copy link
Contributor

langz commented Nov 22, 2024

Tested it quickly on my mobile. Not sure what I think of the order of the buttons. Not that I have any strong opinions, but I somehow expect cancel/avbryt to be at the bottom? Not entirely sure, but I feel there's something off with the existing order here.

image

@@ -205,7 +205,7 @@ export const ArrayFromFormHandler = () => {
</Field.Composition>

<Iterate.Toolbar>
<Iterate.RemoveButton />
<Iterate.RemoveButton confirmRemove />
Copy link
Contributor

@langz langz Nov 22, 2024

Choose a reason for hiding this comment

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

Suggested change
<Iterate.RemoveButton confirmRemove />
<Iterate.RemoveButton confirmRemove />

Can the prop just be named confirm? As the component is RemoveButton, so we sort of know the confirming should be about removing. Hence the name of the component.

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes 👍

Copy link
Member Author

@tujoworker tujoworker Nov 23, 2024

Choose a reason for hiding this comment

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

But on the other hand I like more unique props so it's easier to maintain for everyone.

Could be showConfirmDialog or confirmBeforeRemove?

Any thoughts about such prop names?

Copy link
Contributor

Choose a reason for hiding this comment

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

Sure, perhaps just showConfirmDialog, I think I like that the best.

@tujoworker
Copy link
Member Author

Not entirely sure, but I feel there's something off with the existing order here.

It's a custom toolbar. So apps would need to adjust it to their needs.
I just used Flex.Horizontal. Changing the order is possible, but would need more code in the example. Which is based on @andlbrei needs.

@langz
Copy link
Contributor

langz commented Nov 23, 2024

Not entirely sure, but I feel there's something off with the existing order here.

It's a custom toolbar. So apps would need to adjust it to their needs. I just used Flex.Horizontal. Changing the order is possible, but would need more code in the example. Which is based on @andlbrei needs.

I see, then it's fine by me :)

@langz
Copy link
Contributor

langz commented Nov 23, 2024

https://eufemia-git-feat-forms-confirm-remove-eufemia.vercel.app/uilib/extensions/forms/Iterate/Array/demos/#customize-the-view-and-edit-containers

Some strange animation happening.
The animated item when opening is a bit stuttering, and when closing(avbryt), I'm able to see two items for the one item 🤔

Probably not related to exactly this RemoveButton PR, but yeah.

Screen.Recording.2024-11-23.at.08.14.24.mov

Here's a few screenshots from the video above when opening/closing:

image
image

@langz
Copy link
Contributor

langz commented Nov 23, 2024

In mobile using Safari:

This is probably difficult to resolve. But when removing using the confirm dialog, it sort of laggs/stutters. Sort of feels like it is under heavy load:

trim.1D57F9E8-33ED-486C-B15D-1EC92E09072F.MOV

@tujoworker tujoworker changed the title feat(Forms): add confirmRemove to Iterate.RemoveButton feat(Forms): add showConfirmDialog to Iterate.RemoveButton Nov 23, 2024
@tujoworker tujoworker force-pushed the feat/forms-confirm-remove branch from 15877fe to 93537ea Compare November 23, 2024 19:58
@tujoworker
Copy link
Member Author

This is probably difficult to resolve. But when removing using the confirm dialog, it sort of laggs/stutters. Sort of feels like it is under heavy load:

Its not 100% perfect, but I think the latest commit enhances the UX. Let me know your result.

@tujoworker
Copy link
Member Author

Before we merge this, we should merge #4329

@langz
Copy link
Contributor

langz commented Nov 24, 2024

This is probably difficult to resolve. But when removing using the confirm dialog, it sort of laggs/stutters. Sort of feels like it is under heavy load:

Its not 100% perfect, but I think the latest commit enhances the UX. Let me know your result.

It felt better now, thanks👏

@langz
Copy link
Contributor

langz commented Nov 24, 2024

Before we merge this, we should merge #4329

Sure, I let you handle this🫡

@langz
Copy link
Contributor

langz commented Nov 24, 2024

https://eufemia-git-feat-forms-confirm-remove-eufemia.vercel.app/uilib/extensions/forms/Iterate/Array/demos/#customize-the-view-and-edit-containers

Some strange animation happening.

The animated item when opening is a bit stuttering, and when closing(avbryt), I'm able to see two items for the one item 🤔

Probably not related to exactly this RemoveButton PR, but yeah.

Screen.Recording.2024-11-23.at.08.14.24.mov

Here's a few screenshots from the video above when opening/closing:

image
image

I still experience this. Not sure if this is important to fik, but to me the animation feels a bit strange because it's a few spøit seconds during the animation where I can see both Avatars, as seen in the video:

trim.7DF4AA91-C1C5-41A7-8DFF-8A56D6646FEA.MOV

@tujoworker
Copy link
Member Author

It's not a new thing. But it's more visible with the Avatar now.

That's how we transition the two containers. But this can for sure be improved. We could hide the "hiding" content earlier with an opacity. But in another PR.

@tujoworker tujoworker force-pushed the feat/forms-confirm-remove branch from 93537ea to 2bc90cf Compare November 25, 2024 11:45
@tujoworker tujoworker merged commit 76bddf0 into main Nov 25, 2024
10 checks passed
@tujoworker tujoworker deleted the feat/forms-confirm-remove branch November 25, 2024 13:25
tujoworker pushed a commit that referenced this pull request Nov 29, 2024
## [10.58.0](v10.57.1...v10.58.0) (2024-11-29)

### ✨ Features

* **Forms:** add `showConfirmDialog` to Iterate.RemoveButton ([#4330](#4330)) ([76bddf0](76bddf0))
* **Forms:** add `variant="filled"` and `toolbarVariant="custom"` to Iterate.EditContainer and Iterate.ViewContainer ([#4329](#4329)) ([b2b9eef](b2b9eef))
* **Forms:** add support for using a function references instead of a string based id ([#4331](#4331)) ([a6e3bc3](a6e3bc3))
* **Forms:** enhance typing and add docs on how to deal with TypeScript types ([#4343](#4343)) ([10b199b](10b199b))
* **Forms:** introduce `decoupleForm` prop to Form.Handler ([#4332](#4332)) ([0b02b6e](0b02b6e))

### 🐛 Bug Fixes

* **DatePicker:** make sure the picker and input only reacts to the props that have changed ([#4342](#4342)) ([4cd52a3](4cd52a3))
* **Forms.Card:** remove outline when variant="basic" on Section containers when used in Wizard ([#4336](#4336)) ([ebad212](ebad212))
* **forms:** add `sessionStorageId` support to Field.Upload with empty file list rendering ([#4339](#4339)) ([d02a0af](d02a0af))
* **NumberFormat:** improve regex for parsing phone numbers with country codes ([#4340](#4340)) ([96613ed](96613ed)), closes [#4337](#4337)
@tujoworker
Copy link
Member Author

🎉 This PR is included in version 10.58.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