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

Web update #1

Closed
wants to merge 3 commits into from
Closed

Web update #1

wants to merge 3 commits into from

Conversation

Zackh1998
Copy link

Updated the design of the Offix Web page, and added gulp to compile the scss and make development easier.

@ehzhang
Copy link

ehzhang commented Nov 9, 2016

Can you post some screenshots? :)

@@ -0,0 +1,283 @@
body {
font: 16px "Lucida Grande", Helvetica, Arial, sans-serif;
line-height: 16px;
Copy link

Choose a reason for hiding this comment

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

nit: indentation!

Choose a reason for hiding this comment

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

+1. might be worth running this file through a code beautifier.

}
if (!req.body.username || req.body.username.length < config.MIN_USERNAME_LENGTH) {
// TODO nicer error message
return res
.status(400)
.send('Username too short, min length ' + config.MIN_USERNAME_LENGTH);
.render('create',{errMes:'Username too short, Min. length ' + config.MIN_USERNAME_LENGTH});
Copy link

Choose a reason for hiding this comment

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

nit (code-style): spaces after commas ('create', {...

Choose a reason for hiding this comment

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

+1

@ehzhang
Copy link

ehzhang commented Nov 9, 2016

Also, hi! :P

Happy to see people contributing!!

@anishathalye
Copy link

Hi Edwin! 😄

Copy link

@anishathalye anishathalye left a comment

Choose a reason for hiding this comment

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

Thanks for the pull request. The new style looks awesome!

Screenshots for Edwin / anyone else:

screen shot 2016-11-10 at 3 37 15 pm

screen shot 2016-11-10 at 3 37 23 pm


In addition to the inline comments, here are some general comments on the PR:

  • Aren't favicons supposed to be .ico files?
  • Could we change the <<Back to < Back?

}
if (!req.body.username || req.body.username.length < config.MIN_USERNAME_LENGTH) {
// TODO nicer error message
return res
.status(400)
.send('Username too short, min length ' + config.MIN_USERNAME_LENGTH);
.render('create',{errMes:'Username too short, Min. length ' + config.MIN_USERNAME_LENGTH});

Choose a reason for hiding this comment

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

+1

return res.status(401).send('Invalid signup key');
return res
.status(401)
.render('create',{errMes:'Invalid signup key'});

Choose a reason for hiding this comment

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

code style: space after commas and : (in general, throughout all the code)

}
if (!req.body.password || req.body.password.length < config.MIN_PASSWORD_LENGTH) {
// TODO nicer error message
return res
.status(400)
.send('Password too short, min length ' + config.MIN_PASSWORD_LENGTH);
.render('create', {errMes: 'Password too short, Min. length ' + config.MIN_PASSWORD_LENGTH});

Choose a reason for hiding this comment

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

"Min" -> "min" ?

@@ -0,0 +1,78 @@
// PACKAGES //

Choose a reason for hiding this comment

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

can we get rid of the gulpfile? Most of the stuff in here is unnecessary.

The only thing that looks necessary is the scss compilation. Instead of ahead-of-time compilation, can we use this: https://www.npmjs.com/package/node-sass-middleware ?

@@ -0,0 +1 @@
body{font:16px "Lucida Grande", Helvetica, Arial, sans-serif;line-height:16px;background-color:#000;color:white;text-align:center}.center{margin:auto;max-width:500px;margin-top:40px;margin-bottom:100px}.admin-center{width:85%;max-width:1200px}.admin-center .positive{border:2px solid black}.offix{font-size:7.750em;font-family:'Montserrat', 'Open Sans', arial;color:#5083BF;margin:0px}.offix span{color:#fff;font-weight:500}.title{margin:40px;font-size:2.5em}.error{font-size:1.5em;margin:1em;color:red}.form-group{padding:5px 0px}.input-group-addon{padding:6px 12px;font-size:.875em;color:#888;background-color:transparent;border:0px}.form-control{border:0px;border-radius:0;font-size:1em;height:auto;border-bottom:.063em solid #888;background-color:transparent;color:white}.form-control:focus{box-shadow:none;border-bottom:.063em solid white}.login,.register{display:inline-block;vertical-align:text-bottom;width:auto !important;margin:0px 10px;padding:11px 44px;font-size:.688em;line-height:2em;letter-spacing:.08em;text-transform:uppercase;transition:color .2s, background-color .2s, border .2s}.login{color:black;background-color:#ddd}.login:hover{background-color:#999}.register{color:white;background-color:transparent;border:.063em solid #555}.register:hover{color:#ccc;border:.063em solid #ccc}a{color:#00B7FF;cursor:pointer}hr{border-top:.063em #aaa solid}table{margin:0px auto;height:100%;width:100%;color:#ccc}table #table-titles{text-transform:uppercase}th,td{padding:15px 0px;border-left:.125em black solid;border-right:.125em black solid}tr.alternating:nth-child(even){background-color:#2a2a2a}.toggle-button{padding:0}.toggle-button input{width:100%;height:100%;transition:background-color .2s;background-color:transparent;border:0px}.toggle-button input:focus{outline-color:transparent}.remove-button{margin-left:10px;height:100%;font-size:1.25em;vertical-align:middle;color:#bb3d30;transition:color .2s;cursor:pointer}.remove-button:hover{color:red}.remove-button form{position:absolute;width:100%;height:100%;top:0;left:0}.remove-button input{width:100%;height:100%;background-color:transparent;border:none}tr.positive input:hover{background-color:#158948}tr.negative input:hover{background-color:#953b32}.mac{line-height:1.5em}td.positive,tr.positive,tr.negative{color:white}td.positive,tr.positive{background-color:#259958}td.negative,tr.negative{background-color:#bb3d30}@media only screen and (min-device-width: 320px) and (max-device-width: 1100px) and (-webkit-min-device-pixel-ratio: 2){body{font-size:32px;line-height:32px}td{padding:40px 0px}.offix{font-size:6em}.input-group-addon{font-size:1.2em}.form-control{font-size:1.8em}.register{margin:40px;font-size:1em;border:.15em solid #555;padding:20px 60px;letter-spacing:0}.login{margin:40px;font-size:1em;border:.1em solid #ddd;padding:20px 60px;letter-spacing:0}.center{width:95%;max-width:none;padding:40px 30px 0px 30px}.admin-center{width:95%;max-width:none}.back{display:inline-block;font-size:1.3em;margin-top:10px;margin-bottom:4em}}@media only screen and (min-device-width: 700px) and (max-device-width: 1100px) and (-webkit-min-device-pixel-ratio: 2){body{font-size:18px;line-height:18px}}

Choose a reason for hiding this comment

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

build artifacts shouldn't be checked in. could you remove this and add it to the gitignore?

@@ -0,0 +1,283 @@
body {
font: 16px "Lucida Grande", Helvetica, Arial, sans-serif;
line-height: 16px;

Choose a reason for hiding this comment

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

+1. might be worth running this file through a code beautifier.

@@ -3,7 +3,8 @@
"private": true,
"license": "GPL-3.0",
"scripts": {
"start": "node ./bin/www"
"start": "node ./bin/www",
"dev": "gulp"

Choose a reason for hiding this comment

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

can get rid of this once the gulpfile is gone

@@ -22,6 +23,9 @@
"serve-favicon": "~2.3.0"
},
"devDependencies": {
"readline": "^1.3.0"
"readline": "^1.3.0",
"gulp": "^3.9.0",

Choose a reason for hiding this comment

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

can remove gulp stuff

link(rel='stylesheet', href='/stylesheets/style.css')
link(rel="stylesheet",
href="https://maxcdn.bootstrapcdn.com/bootstrap/3.3.7/css/bootstrap.min.css",
integrity="sha384-BVYiiSIFeK1dGmJRAkycuHAHRg32OmUcww7on3RYdg4Va+PmSTsz/K68vbdEjh4u",

Choose a reason for hiding this comment

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

wohoo, SRI 😛

crossorigin="anonymous")
link(href="https://fonts.googleapis.com/css?family=Montserrat|Open+Sans", rel="stylesheet")
link(rel='stylesheet', href='/stylesheets/styles.css')
link(rel="icon", type="image/png", href="img/favicon.png")

Choose a reason for hiding this comment

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

Why not use /favicon.ico?

Copy link
Author

@Zackh1998 Zackh1998 Nov 11, 2016

Choose a reason for hiding this comment

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

I copied the favicon from the techx website, and it used a .png, so I just assumed that it was ok to have .png as a favicon. I've converted it to an .ico though.

@anishathalye
Copy link

Awesome, thanks for the changes!

I made minor changes (got rid of whitespace errors), rebased onto master, squashed into one commit (8e0453f), and merged it in (71348e4)!

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