Add modal for new transactions #631

2024-04-16T06:11:59+08:00

JoshAntBrown commented

2024-04-16 06:11:59 +08:00

(Migrated from github.com)

This change adds the new transaction modal.

https://github.com/maybe-finance/maybe/assets/1793797/8643056f-b81d-4ccf-baa4-f3d21610ce8e

All fields are required:

As part of this I've added a new TurboStreamsRedirect concern to the ApplicationController. This concern adds the ability to specify a turbo_frame on redirect_to, which allows us to decide from the server when to escape out of the modal by setting the turbo_frame of the redirect to _top.

/claim #629
Resolves #629

This change adds the new transaction modal. https://github.com/maybe-finance/maybe/assets/1793797/8643056f-b81d-4ccf-baa4-f3d21610ce8e All fields are required: <img width="1512" alt="Required fields" src="https://github.com/maybe-finance/maybe/assets/1793797/d61ae906-ab09-4b7e-94f7-03cfa93a72a0"> As part of this I've added a new `TurboStreamsRedirect` concern to the ApplicationController. This concern adds the ability to specify a turbo_frame on redirect_to, which allows us to decide from the server when to escape out of the modal by setting the turbo_frame of the redirect to `_top`. /claim #629 Resolves #629

zachgoll (Migrated from github.com) reviewed 2024-04-17 02:40:48 +08:00

zachgoll (Migrated from github.com) left a comment

@JoshAntBrown nice work on this, functionality looks good here. I'm thinking we go with #633 on this one though.

I'm a little hesitant to merge in the TurboStreamsRedirect concern and redirect.turbo_stream.erb at this stage mainly to keep OSS onboarding as easy as possible for new contributors. I know we lose a little bit of flexibility without it, but I'm generally leaning towards keeping things as native as possible here early on and abstracting away things like this when the feature-set has matured a little bit.

@JoshAntBrown nice work on this, functionality looks good here. I'm thinking we go with #633 on this one though. I'm a little hesitant to merge in the `TurboStreamsRedirect` concern and `redirect.turbo_stream.erb` at this stage mainly to keep OSS onboarding as easy as possible for new contributors. I know we lose a little bit of flexibility without it, but I'm generally leaning towards keeping things as native as possible here early on and abstracting away things like this when the feature-set has matured a little bit.

JoshAntBrown commented

2024-04-17 02:44:41 +08:00

(Migrated from github.com)

No worries, I thought that might be the case.

The concern is pretty light weight though and at some point I imagine you'll need a solution for forms where validation may need to be raised. It'll be here to reference if that comes up though.

No worries, I thought that might be the case. The concern is pretty light weight though and at some point I imagine you'll need a solution for forms where validation may need to be raised. It'll be here to reference if that comes up though.

👍 1

zachgoll commented

2024-04-17 02:56:00 +08:00

(Migrated from github.com)

@JoshAntBrown yeah this issue definitely has me thinking a bit more about forms / UI patterns with the modals. I'm almost thinking we try and keep forms in modals lightweight on the validation and then for forms where we do heavy server-side validation we build out full-page flows that have a little more forgiveness on the streams interactions. I think the CSV Transaction import flow we'll be introducing soon might be a good example of this, but tbd...

I do agree though, this is inevitable as some point 😅

@JoshAntBrown yeah this issue definitely has me thinking a bit more about forms / UI patterns with the modals. I'm almost thinking we try and keep forms in modals lightweight on the validation and then for forms where we do heavy server-side validation we build out full-page flows that have a little more forgiveness on the streams interactions. I think the CSV Transaction import flow we'll be introducing soon might be a good example of this, but tbd... I do agree though, this is inevitable as some point 😅

👍 1

Pull request closed

This pull request cannot be reopened because the branch was deleted.

Sign in to join this conversation.

1 Participants

Notifications

Due Date

No due date set.

Dependencies

No dependencies set.

Reference: gavin/maybe#631