-
Notifications
You must be signed in to change notification settings - Fork 14
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
Add the Game Gig event page #113
Add the Game Gig event page #113
Conversation
There is currently no styling or a bunch of other features. 😭 However, the skeleton of an awesome event page is there! I have followed the general layout of the one from last year - https://gamegig.hackersatcambridge.com/, but have tried to |
The JavaScript that updates instances of CurrentTime work on an id basis, instead of a class basis.
Add a mechanism to include custom css for specific events.
fa1f6cf
to
c7b6695
Compare
…ite into feature/game-gig
Done so that when we display them, order is preserved.
…ite into feature/game-gig
CountDownTimer now uses that instead of inline JavaScript in string literals.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could also use DictionaryLiteral to preserve the order
@jaredkhan Didn't know about that, that is an option. EDIT |
@moosichu just pushed some changes for ya |
@Pinpickle @jaredkhan @CharlieCrisp @eliotlim @martinhartt I know feel that this is in a state to be merged! Please review ASAP! |
const endDate = {{endDate}}; | ||
const CountDownTimerId = {{id}}; | ||
const CountDownTimerPreId = {{preId}}; | ||
const beforeEventMessage = {{beforeEventMessage}} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let's be consistent with semicolons
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Worth noting that we're going semicolonless in our gulpfile, following this standard
(up for discussion on this, can probably wait until later)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Had a pass over the code. Great work
Also, don't need gamegig3000-logo.png
anymore
import Foundation | ||
|
||
struct CountDownTimer : Nodeable { | ||
let startDate : Date |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
To me this reads like the date that the timer starts counting down. If this is specific to events, maybe we could use some /// doc comments on this struct
let endDate : Date | ||
let id = "CountDownTimer\(UUID().description)" | ||
|
||
let preId = "CountDownTimerPre\(UUID().description)" // the id of the countdown message |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Don't love that we are changing the ID of an element. I expect we can get away without this
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I have created issue #170 for this, I think fixing this is low priority.
].containing("YOU SHOULD SEE THE TIME REMAINING HERE"), | ||
Script( | ||
file: "Hackathons/CountDownTimer.js", | ||
escapes: [ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not sure 'escapes' is the right word to be using here. 'context' is a common alternative.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
How come? I think 'context' is more confusing than 'escapes'.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The current system is a half-way house anyway tbh.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What about 'definitions'?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good shout.
("Web Dev with Mozilla", "https://globalgamejam.org/news/dev-web-mozilla") | ||
] | ||
|
||
let GoboLogo = El.A[Attr.href => "http://studiogobo.com/", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We should lowercase variables/constants
// swiftlint:disable line_length | ||
|
||
|
||
extension String { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice! We could move this out to somewhere else. Files for extensions conventionally take the form: e.g. String+idMangle.swift
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Created issue #171 for this.
import DotEnv | ||
import Foundation | ||
|
||
protocol JavaScriptable { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This could definitely use a doc comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
OKIEDOKIE
/** | ||
* This class is used in order load front-end scripts from a file relative to the current path for browsing pleasure. | ||
* | ||
* It depends on the build system loading files into the 'Data' folder before running. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Does it?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Your right, that has been changed! Will fix. Good catch.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Still this @moosichu
Sources/HaCWebsiteLib/routes.swift
Outdated
@@ -17,7 +17,7 @@ func getWebsiteRouter() -> Router { | |||
"/git": "https://github.com/hackersatcambridge/git-workshop-2017", | |||
"/binary-exploitation": "https://github.com/hackersatcambridge/binary-exploitation/blob/master/handout.md", | |||
"/make-games-with-love": "https://github.com/hackersatcambridge/make-games-with-love", | |||
"/gamegig": "https://www.facebook.com/events/124219834921040/" | |||
"/gamegig": "/event/2017/gamegig3000" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why the year if we also have a version number in the name?
@@ -40,10 +40,13 @@ function buildStatic () { | |||
main: './main.js', | |||
// Styles entry points | |||
'styles/main': './styles/main.styl', | |||
// TODO: Ideally we want this to be a wildcard match of some form | |||
// so we can automatically pick-up new custom styles. | |||
'styles/custom/gamegig2017': './styles/custom/gamegig2017.styl' |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could do with an issue for this
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is there a GitHub thing called a debate
? ;)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Created #173 for this.
a { | ||
text-decoration: none; | ||
padding: none; | ||
--link-color: #fd7cb3; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What's this syntax?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The hot new thing ;)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is it worth using these over less stylus variables?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I honestly don't have an opinion either way, I think it's probably worth discussing with @Pinpickle .
I'm just not knowledgable enough.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We got a bunch of tradeoffs here!
For CSS Custom Properties:
- An official thing, so people get to learn something applicable for every project as opposed to those with preprocessors
- You can set them on a per selector basis. On one link I can set
--link-color
to one thing, and on another I can set it to something else and they won't conflict. - Can set them dynamically in JS or CSS pseudo-classes (like
:hover
) and changes will propagate (+++) - You can give them values before or after you use them, putting less importance on file ordering
Against:
- Browser support is getting better, but not fully there: https://caniuse.com/#feat=css-variables (notable example is IE 11 but I think this is fine for our userbase)
- We now have another decision to make when making a variable: Should this be in Stylus or in CSS?
- Fair bit more verbose than Stylus
- Have to go "all in" on CSS when using them. So we can't use Stylus operators (have to use
calc
) or colour functions (these are coming to CSS but are basically a dream at the moment)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we please come to a conclusion on what to do in this instance? The debate is interesting, but I want to get this live ASAP.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Probably best to stick with Stylus for now to avoid changing things?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Currently I'm using CSS vars I think, so change those to stylus ones yes?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That's the idea! To clarify I meant avoiding changing the convention in our codebase
@jaredkhan All your concerns should be addressed now. :) |
@jaredkhan It should all really be addressed now! I would really like this live today, so that the banner is on the site as soon as this workshop is done. |
The comments were outdated and so that part has been removed.
467f564
to
5ec4388
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let's gig
Adds a page for the Game Gig event. Still massively in progress!!!
Current state:
Fixes #113 and #135.