search

savvato-software / dtim-mobile

The application we use to interact with people during a tech interview meeting.
1 stars 3 forks source link

Added email to returning user form #25

Open ethan375 opened 4 years ago

haxwell commented 4 years ago

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 #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 commented 4 years ago

Hey Johnathan,

Thank you for the feedback,

  1. I will put a line under both of those elements.
  2. I will look at his work and incorporate it 3.Okay I will change it back to concat, do we have any documentation on standards?
  3. It passes on my machine! lol, I will change those to use the data attributes

I dont have work today, I will work on this today and get back to you!

On Sun, Mar 29, 2020 at 9:53 PM Johnathan James notifications@github.com wrote:

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 https://github.com/braydenc303 added some email validation in his work in PR #24 https://github.com/savvato-software/dtim-mobile/pull/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

— You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub https://github.com/savvato-software/dtim-mobile/pull/25#issuecomment-605768314, or unsubscribe https://github.com/notifications/unsubscribe-auth/AGIGD7JI2CV4M2FCFAIKM3TRKAJUFANCNFSM4LWC7MQA .

ethan375 commented 4 years ago

new pr working on validation issues for returning user