SPAC2020 #6
SPAC2020 #6
Conversation
|
In the future, please split these in to multiple logical commits to help me review the content faster. EDIT: I will actually ask that the commit description is changed to something along the lines of "Front-end overhaul for the 2020 year" |
|
I believe those are very old commits. I don’t know how they showed up in the pull request. |
|
|
Reopened. |
|
@dynamic11 No, the file changes are fine, but they are only included in one commit. This makes reviewing piece-by-piece much trickier. Most of the deletions are fine, but may warrant a re-write of history after this year to remove lots of the old binaries. |
|
@webmaster-ieeecarleton |
webmaster-ieeecarleton
left a comment
webmaster-ieeecarleton
left a comment
There was a problem hiding this comment.
This is a review of the first 97 changes. I will review the bigger additions piecewise.
| @@ -1 +0,0 @@ | |||
| ieeespac.ca No newline at end of file | |||
There was a problem hiding this comment.
The CNAME record is required for github's DNS to reflect that the *.github.io are alias domains. Please remove this change.
.travis.yml
Outdated
| @@ -0,0 +1,14 @@ | |||
| language: node_js | |||
There was a problem hiding this comment.
I don't believe we currently have travis enabled. Please make a new pull request for travis specifically, if you'd like to add CI.
.gitignore
Outdated
| @@ -0,0 +1,3 @@ | |||
| node_modules | |||
| bower_components | |||
| .idea/workspace.xml | |||
There was a problem hiding this comment.
This specific include should be part of the local gitignore. Please remove line 3.
|
|
||
|
|
||
|
|
||
| The MIT License (MIT) Copyright (c) 2017 markups.io |
There was a problem hiding this comment.
Why was the license removed?
(Please update the copyright header!)
| @@ -1,2 +0,0 @@ | |||
| # SPAC2018Template | |||
There was a problem hiding this comment.
Could you please update the readme instead of deleting it?
|
@flora8984461, maybe create a feature branch and the other two branches I mentioned to you earlier. But maybe work on this repository? Here is what I outlined earlier. @webmaster-ieeecarleton what do you propose? |
webmaster-ieeecarleton
left a comment
webmaster-ieeecarleton
left a comment
There was a problem hiding this comment.
CSS-fonts: looks good except the dummy text file
| @@ -1 +0,0 @@ | |||
| dummy.txt | |||
There was a problem hiding this comment.
Can this file be removed entirely? I'm not sure why it's here.
There was a problem hiding this comment.
I am sorry I am not sure why this file shows up, but it is not in my repo.
|
@rafiddewan mind pulling that discussion into an issue? its not something that should be done in the next 12 hours. |
webmaster-ieeecarleton
left a comment
webmaster-ieeecarleton
left a comment
There was a problem hiding this comment.
For the images: any image > 100 KB should be checked if further compression is possible. Images > 1 MB are unacceptable. The loading time for some of these images will be poor. I'll accept them due to the deadline, but these need to be addressed in another update after this is merged ASAP.
| -------------------------------------------- | ||
| Bootstrap : | ||
| Website URL : http://getbootstrap.com/ |
There was a problem hiding this comment.
These credits were correctly removed in the original commit. Not a blocker.
|
Looks good overall. I will test tomorrow morning and merge if all is well after I get some sleep 💤 |
|
@webmaster-ieeecarleton |
4 participants
No description provided.