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

BREAKING CHANGE: add forgot password feature in the SupaEmailAuth widget #46

Merged
merged 13 commits into from May 22, 2023

Conversation

dshukertjr
Copy link
Member

@dshukertjr dshukertjr commented Mar 16, 2023

What kind of change does this PR introduce?

This PR combines the forgot email flow, signIn, and signUp into a single widget so that the developer using this library does not have to separately implement it.

The aim for this PR is to make it more similar to how Supabase auth UI for react is implemented.

This PR adds the following

  • Add forgot password feature
  • Remove authAction parameter on SupaEmailAuth widget.
    • Now there is a button to toggle between signIn and signUp in SupaEmailAuth widget, so that there is no need for developers to have two separate SupaEmailAuth in two different places.
  • Minor internal variable renaming for some cleanup

With this PR, you can have this code

SupaEmailAuth(
  onSignInComplete: (response) {
    // handle signIn
  },
  onSignUpComplete: (response) {
    // handle signUp
  },
),

To get this:
Mar-16-2023 14-52-19

@dshukertjr dshukertjr changed the title Breaking: add forgot password feature in the SupaEmailAuth widget [Proposed] Breaking: add forgot password feature in the SupaEmailAuth widget Mar 16, 2023
@dshukertjr dshukertjr changed the title [Proposed] Breaking: add forgot password feature in the SupaEmailAuth widget Proposed Breaking: add forgot password feature in the SupaEmailAuth widget Mar 16, 2023
@dshukertjr
Copy link
Member Author

@FatumaA @Csierram96
I wanted to create this PR as a proposal, and I would like to hear what you think about it. Obviously it is quite a huge change from the current limitation, and there are pros and cons about this implementation. Any comments is appreciated, and I don't intend to merge this if you think this is not a good idea, so please don't hesitate to let me know if you don't like it if you don't!

@dshukertjr
Copy link
Member Author

Will do additional cleanup of example directory if we think this PR is a good idea.

@FatumaA
Copy link
Collaborator

FatumaA commented Mar 16, 2023

Seems like the next logical step 🙂. I will take a closer look a little later today.

Thank you for continuing to maintain this while I could not!

@Csierram96
Copy link
Contributor

I'll also take a look by eod. But by looking at the description and the video looks like a nice change/feature

Copy link
Collaborator

@FatumaA FatumaA left a comment

Choose a reason for hiding this comment

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

I think switching the '/' from SignIn to home is not necessary since it is an auth lib. I'm assuming that the sign in screen is what should be at the root?
If we keep that change we have to make minor changes on how the app switches screen

Otherwise it looks fine 👍🏽
I will add a commit to remove the forgot password text as it shows doubly

also, do we still need the update_password screen in the example?

I have noticed a bug on validation: fields stay in error even when you toggle between the screens in supaEmailAuth

Remove the forgot password and don't have account text buttons from the example
///
/// Typically used to pass a DeepLink
final String? redirectUrl;
/// If email confirmation is turned on, the user is
Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe I'm reading this wrong, but I dont get this description. The user is, is there something missing here?

Copy link
Member Author

Choose a reason for hiding this comment

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

This is very much a typo! Thanks for finding it, and I will fix it!

MapEntry(metaDataField.key, controller.text)),
);
)
: Text(_isSigningIn ? 'Sign In' : 'Sign Up'),
Copy link
Contributor

Choose a reason for hiding this comment

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

not related to this PR but would be nice to be able to modify the text that shows up. This would be useful to handle intl

Copy link
Member Author

Choose a reason for hiding this comment

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

Yup, for sure! Certainly something we would want to do in the near future!

@Csierram96
Copy link
Contributor

It looks like a really good addition. Just curious, what are the cons that you see of adding this?

@dshukertjr
Copy link
Member Author

Thanks for all the positive comments!

@FatumaA

I think switching the '/' from SignIn to home is not necessary since it is an auth lib. I'm assuming that the sign in screen is what should be at the root?
If we keep that change we have to make minor changes on how the app switches screen

Thanks for the feedback! Makes sense! Will put it back the way it used to be!

Otherwise it looks fine 👍🏽
I will add a commit to remove the forgot password text as it shows doubly

also, do we still need the update_password screen in the example?

I have noticed a bug on validation: fields stay in error even when you toggle between the screens in supaEmailAuth

Yup, I haven't actually checked the example app thoroughly yet. I just wanted to show this feature to you to get some feedback! I will do some cleanup of the example directory!

@dshukertjr
Copy link
Member Author

Thanks for the positive feedback @Csierram96 !

It looks like a really good addition. Just curious, what are the cons that you see of adding this?

I guess I had two things:

  • We just had a fairly large breaking change, and this feature is another large breaking change. I know the library is still in v0.x, which means it's still in pre-release mode therefore breaking change is okay, but wanted to get a feel on how you felt about it.
  • Less flexibility. Before this update, developers were able to have login and sign-up page in two different routes, but now with this update, login and sign-up are in the same widget, so they have to be in the same page. There are some flexibility lost with this update. I guess though at the end, developers will use this supabase_auth_ui to get up and running quickly, so it might be a bad thing to have less flexibility and more to offer out of the box. If they want flexibility, they can always just create their own login and sign up form from scratch.

@Csierram96
Copy link
Contributor

Thanks for the positive feedback @Csierram96 !

It looks like a really good addition. Just curious, what are the cons that you see of adding this?

I guess I had two things:

  • We just had a fairly large breaking change, and this feature is another large breaking change. I know the library is still in v0.x, which means it's still in pre-release mode therefore breaking change is okay, but wanted to get a feel on how you felt about it.
  • Less flexibility. Before this update, developers were able to have login and sign-up page in two different routes, but now with this update, login and sign-up are in the same widget, so they have to be in the same page. There are some flexibility lost with this update. I guess though at the end, developers will use this supabase_auth_ui to get up and running quickly, so it might be a bad thing to have less flexibility and more to offer out of the box. If they want flexibility, they can always just create their own login and sign up form from scratch.

That makes sense. The braking change should be fine as you mentioned as this has not been released as stable. And regarding the flexibility that's actually a good point. Personally I prefer something that just works out of the box for this kinda stuff (firebase ui or supabase ui). And if you want more flexibility you can implement your own UI.

@dshukertjr
Copy link
Member Author

Thanks @Csierram96 for the nice comment! It's nice to receive a sanity check from other developers! I will do some additional clean up on this PR and merge it in!

@dshukertjr dshukertjr changed the title Proposed Breaking: add forgot password feature in the SupaEmailAuth widget BREAKING CHANGE: add forgot password feature in the SupaEmailAuth widget May 22, 2023
@dshukertjr dshukertjr requested a review from FatumaA May 22, 2023 02:30
@dshukertjr
Copy link
Member Author

dshukertjr commented May 22, 2023

I think this one is good to be reviewed!

@dshukertjr dshukertjr merged commit d388de6 into main May 22, 2023
1 check passed
@dshukertjr dshukertjr deleted the BREAKING/forgot-password branch May 22, 2023 07:15
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants