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

Changed validation when creating email #1827

Merged
merged 5 commits into from
May 1, 2024
Merged

Conversation

jzbahrai
Copy link
Contributor

@jzbahrai jzbahrai commented Apr 30, 2024

Summary | Résumé

Changed how we validate when you create a service email. We won't require you to click continue and then recheck the data

Screenshot 2024-04-30 at 4 42 03 PM

https://app.zenhub.com/workspaces/notify-planning-614b3ad91bc2030015ed22f5/issues/gh/cds-snc/notification-planning/1473

Test instructions | Instructions pour tester la modification

  1. Create a service
  2. Use an email address that has spaces and weird characters
  3. See the above message show up before you go to the next screen
  4. Check the email address is correct in the settings page

Copy link

Copy link
Member

@andrewleith andrewleith left a comment

Choose a reason for hiding this comment

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

This looks great! A couple of minor suggestions below.

tests/app/main/views/test_add_service.py Show resolved Hide resolved
Copy link
Contributor

@amazingphilippe amazingphilippe left a comment

Choose a reason for hiding this comment

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

Just one small detail! You'll also need to compile tailwind.

app/templates/components/information-box.html Outdated Show resolved Hide resolved
app/templates/components/information-box.html Outdated Show resolved Hide resolved
Copy link
Contributor

@amazingphilippe amazingphilippe left a comment

Choose a reason for hiding this comment

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

I went in and did some changes to the macro and partial markup. We had too many nested divs!! 😵
image

Copy link
Contributor

@amazingphilippe amazingphilippe left a comment

Choose a reason for hiding this comment

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

Looks good to me, but since I committed some stuff, lets also get @andrewleith 's review

Copy link
Member

@andrewleith andrewleith left a comment

Choose a reason for hiding this comment

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

LGTM! Nice work!! :shipit:

@jzbahrai jzbahrai merged commit 0bafe65 into main May 1, 2024
9 checks passed
@jzbahrai jzbahrai deleted the task/autocorrect-warning branch May 1, 2024 19:20
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants