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

Upon signup with email, we now warn about email in terminal in Dummy email provider is used. #2052

Closed
wants to merge 1 commit into from

Conversation

Martinsos
Copy link
Member

This PR was inspired as a possible solution to wasp-lang/open-saas#137 -> give it a read, it is important for the context.

Result is this, upon successful signup:

image

I could have added the same thing for other email auth forms (password reset, ...) but felt that might be a bit much.

I am not even sure about this. It seems to me like it will help people on the happy path, when they are creating their first account, and can't find that email in the email. Solution can also be just making it clearer in the docs, putting warnings about it in the right place, and that might be enough. For example, once they are ready to start doing something with their open-saas app, we can mention in the guide that Dummy provider is used and they will not receive emails.

But I wanted to try this. At the end, this is the best way to inform the dev.

The problem is, there are other situations where Dummy provider might surprise them, e.g. if job is sneidng email or something. And we have no warning in that case. We just can't in every case inform them about this. Nor should we , it would be annoying. So I though ok, this is "entry point" and likely if they will get confused, it will happen here, so we can teach them here.

I am ok with dropping this PR if we conclude it is more harm then good, but if others like it, we can merge it.

setSuccessMessage(
`You've signed up successfully! Check your email for the confirmation link.`
{=# isEmailSenderProviderSetToDummy =}
+ ` (NOTE(DEV): Your Wasp app is using Dummy email provider, which doesn't really send emails: instead, check server logs in the terminal for the email)`
Copy link
Contributor

@sodic sodic May 23, 2024

Choose a reason for hiding this comment

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

I get the issue, I like the idea, and I think this is a good approach... but I'm not a fan of this message 😄

I'm basing my feedback on the screenshot, so keep that in mind if something's changed since you created it.

Since this is something all users will see, I think we should make it as high-quality as reasonably possible.

Here's what I'd improve:

  • Make an entirely new message box (if that's easily done) that's in a different color and shows the dummy message.

  • If not, make the text less cluttered by removing the double parentheses and putting the dummy message in a separate new line (maybe even with a line in between).

  • Put the dummy email text in italics or something similar (slightly different color) to make it stand out as something a user wouldn't normally see (again, only if it's easily supported).

  • Improve the wording (by getting rid of filler words), fix grammatical errors (articles mostly), change "email" to "confirmation link" (possibly debatable). @vincanger can weigh in some more, he's a native speaker:

    Suggested change
    + ` (NOTE(DEV): Your Wasp app is using Dummy email provider, which doesn't really send emails: instead, check server logs in the terminal for the email)`
    + `NOTE(DEV): Your app is using the Dummy email provider, which doesn't really send emails. You can find the confirmation link in the server logs`

P.S. Props for attaching a screenshot! It made it much easier for me to give (what I believe is) good feedback.

Copy link
Contributor

@infomiho infomiho May 23, 2024

Choose a reason for hiding this comment

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

I think we should make it as high-quality as reasonably possible.

I agree with this point, I had the same reaction upon seeing the screenshot.

This feels important to me: You should be able to see what your users experience for real and you can write e2e tests that match the production UI. This extra piece of text feels off on those two points.

Since this is all on the client - this means we can design stuff. I'd rather go with a separate box or some sort of a design that indicates this is "dev mode" and then explain what "dev mode" means. I don't have a complete look in my mind, but if you recall how Astro has it's dev toolbar - it's an extra piece of UI on top of the standard UI.

Copy link
Contributor

@sodic sodic May 23, 2024

Choose a reason for hiding this comment

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

I'd rather go with a separate box or some sort of a design that indicates this is "dev mode" and then explain what "dev mode" means.

The first version of my comment suggested the same thing, but I didn't know how to precisely word it and feared it was too big to suggest 😄

But now that the cat's out of the bag, I can say I'm a big fan of this idea!

@Martinsos
Copy link
Member Author

@sodic @infomiho you pplz are right! I didn't want to go too much into design for two reasons:

  1. Wanted to first hear if the whole idea makes sense.
  2. It would be quite bigger change and I wasn't sure if we want so much code for a "minor" thing like this.

But the idea you pplz mentioned, I have been thinking about it while coding this also. And it is a better, more scalable way to do stuff like this.
So ideally, instead of pushing messages like this into the generated code, we would have some kind of a visual thing in the app itself that is able to display warnings / error and similar.

Solution could be whole wasp studio, that runs in another tab, with logs, messages, app overview, ... . But that is far away, and would still not be enough "in your face". So while I think we should have that in some way, it wouldn't solve this particular problem of informing them right now.

What would be faster and possibly complimentary solution is something that is our dev tool thing that is rendered as part of the app, and shows warnings, errors, ... .
I wrote about this some time ago here: #250 (comment) , idea was some kind of small popover that communicates basic stuff.
This is probably similar to the Astro's dev toolbar @infomiho (I haven't used Astro)?

Ok, so I suggest following then:

  1. I will close this PR, it feels too much work to make this pretty compared to doing it in more scalable way right away.
  2. I will create another issue, which will be for creating a very first version of Wasp's dev toolbar, that will be rendered in the app in a minimally disruptive way, and will for start be able to just show messages (errors, warnings). We will then make the first usage of it for this notification about Dummy provider.

This will be an interesting experiment that will quickly bring value, and later as Wasp Studio evolves, we can figure out how to make those two work together or merge them.

@sodic
Copy link
Contributor

sodic commented May 23, 2024

@Martinsos Perhaps it's enough for now if we had a message pop up in the corner of the screen for all the dev stuff (that's pretty popular these days). Tell me if you don't know what I'm referring to (@infomiho will probably know what it's called).

Before we tackle wasp dev tools, wasp studio and whatnot.

@infomiho
Copy link
Contributor

@sodic I believe this "dev toolbar" could be simple to pull off, so maybe it makes sense to go with it instead of a detached effort of adding some sort of a toast 👇

SolarFlow.mp4

@Martinsos
Copy link
Member Author

Seems to me @sodic @infomiho like we are talking about the same thing in different ways hm, but aren't we all proposing the same? Add a very simple mechanism, for start, to show dev messages in their app while they are developing it? It just has to be made in such way that it is clear it is our "dev" thing.

So if we indeed agree about this, then I would do as I suggested above:

  1. Close this issue.
  2. Open another issue for this idea.

@sodic
Copy link
Contributor

sodic commented May 23, 2024

@infomiho @Martinsos I can confirm I was talking about the very thing Miho's video shows. Simple popup messages in the corner.

I have no idea why that's called a toolbar as it has nothing to do with either tools or bars. Developers naming things, I guess 😅

@Martinsos
Copy link
Member Author

Martinsos commented May 24, 2024

Closing, I instead opened #2059 to implement simple wasp dev toolbar.

@Martinsos Martinsos closed this May 24, 2024
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.

3 participants