jperitz-dev #41
jperitz-dev #41
Conversation
There was a problem hiding this comment.
@jperitz thank you for submitting! The login page overall looks good.
I guess the initial issue didn't emphasize this well enough, but this isn't a normal login page. The idea is this is a sign in students will use when they come to club events. As a result we wont be using a password (the backend will just ignore repeated requests). So you can remove the password related elements. It also might make more since to call it a sign-in as opposed to a log-in.
Other than that I had a few questions about styling (specifically why there is Angular stuff in it).
It's a good first pass! Nice job. Look through the other comments I've made and let me know if you have any question.
| @@ -43,3 +43,6 @@ function showSlides(n) { | |||
| dots[slideIndex-1].className += " active"; | |||
| timeOut = setTimeout(() => plusSlides(1), 5000); //Increments slide every 5 seconds automatically, timer is reset whenever a slide is manually selected | |||
| } | |||
|
|
|||
| //Functions for the login page | |||
|
|
|||
There was a problem hiding this comment.
This is something I need to change about the other pages. The plan with the scripts folder is to treat it like the styles folder (so there will be a unified script for the header functions, and then each page would make it's own script if it needs it). I'll make those changes soon, just wanted to let you know.
website/login.html
Outdated
|
|
||
| <div class="field"> <!-- Email field --> | ||
| <label><b> Email</b></label> | ||
| <input type="email" id="email" placeholder="[email protected]" ng-model="email" required class="ng-valid ng-valid-email ng-dirty ng-touched"> |
There was a problem hiding this comment.
I like the place holder with the "ufl.edu" to prompt them to use that email!
website/login.html
Outdated
| </div> | ||
|
|
||
| <div class="field"> <!-- password field --> | ||
| <b><label> Password</label></b> |
There was a problem hiding this comment.
Generally it's best practice to not use <b> in HTML, because it unintentionally changes the styling that CSS applies. You could make a class in the login.css page to make these bold (or whatever else you think makes them look nice).
There was a problem hiding this comment.
Alright cool I didn't know that.
website/projects.html
Outdated
| @@ -20,6 +20,7 @@ | |||
| <!-- Page links --> | |||
| <div class="topnav-right"> | |||
| <div class="topnav-placeholder"></div> | |||
| <a class="topnav-link" href="login.html">Login</a> | |||
There was a problem hiding this comment.
Good thinking adding a link to the topnav (I forgot it in the original issue haha).
website/styles/login.css
Outdated
|
|
||
| /* Full-width inputs */ | ||
| input[type=email], input[type=password] { | ||
| width: 100%; |
There was a problem hiding this comment.
100% width looks fine on mobile, but a little weird on desktop. Consider using media queries to have the properties change on different sized devices.
website/styles/login.css
Outdated
| } | ||
|
|
||
| /* Avatar image */ | ||
| img.avatar { |
There was a problem hiding this comment.
We probably won't be using avatars for the sign in (see my summary comment as to why), so we won't need this.
|
Thanks for the review! Yeah, when I started working on this, I had the requirements in mind, but then I was busy and stepped away for a week and evidently kind of forgot about them. I should have time this weekend to make the changes you suggested. |
Fixes
Added skeleton for the login page front-end with basic CSS.
Updated other html files to reflect changes.
Related Issues
#37