Skip to content
This repository was archived by the owner on Oct 28, 2021. It is now read-only.

Added email to returning user form#25

Open
ethan375 wants to merge 10 commits intosavvato-software:devfrom
ethan375:dev
Open

Added email to returning user form#25
ethan375 wants to merge 10 commits intosavvato-software:devfrom
ethan375:dev

Conversation

@ethan375
Copy link
Copy Markdown

No description provided.

@haxwell
Copy link
Copy Markdown
Contributor

haxwell commented Mar 30, 2020

Thanks for this Ethan... a couple comments..

  1. When running the app, in the returning-user view, it feels like only the Email field has a line under it. Both fields should have a line under them.
  2. @braydenc303 added some email validation in his work in PR Dev #24. It has not yet been merged to our main dev branch, but you can take a look at it to include in the returning-user view as well. I know its a bit of a scope change, but this email field should only accept valid emails (or as near as our email validator can confirm for us.)
  3. In your commit in user.service.ts, you change a string concatenation to interpolation. This is fine, and in fact, interpolation is probably the better choice, but we do concatenation everywhere else. We should be consistent. I am all for a PR changing our use of concatenation to interpolation, but in the meanwhile lets stay with concatenation.
  4. Cypress tests did not pass when I ran them locally. Also, it is better to use the data-* attributes to locate components in the DOM. They are less likely to change, compared to the id field. It seemed like the #phone locator was not finding its component. I think this is because the id as described in the HTML has quotes around it, but the Cypress code does not. (a cursory guess).

+1

@ethan375
Copy link
Copy Markdown
Author

ethan375 commented Mar 30, 2020 via email

@ethan375
Copy link
Copy Markdown
Author

ethan375 commented Apr 1, 2020

new pr working on validation issues for returning user

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants