-
Notifications
You must be signed in to change notification settings - Fork 12
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
narendasan/events and hackillinois #44
narendasan/events and hackillinois #44
Conversation
Just need the nav bar |
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.
Looks pretty good to me (probably wanna fix merge conflicts first), only had a few comments. Did we want to add the events location now, or I can do it in another PR, since I have the events service running locally?
border-left: solid 2px @button-color; | ||
border-right: solid 2px @button-color; | ||
//border-left: solid 2px @button-color; | ||
//border-right: solid 2px @button-color; |
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 reason some of these are commented out and not removed?
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 if I wanted to bring them back, will probably remove
"<div class='event'> \ | ||
<h5><a href='" + event.url + "'>" + event.name + "</a></h5> \ | ||
<ul> \ | ||
<li>" + startDate + "<br/>" + startTime + "</li> \ |
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.
Do we want to add the location of the event as well? I didn't do it before as @tyler-thetyrant never added it, but now he did so the data we're getting now looks like:
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.
Location would be good, have not attempted it, did not want to mess with the event or create a new one since all events are public on the page.
@@ -15,5 +15,5 @@ | |||
display: flex; | |||
justify-content: center; | |||
align-items: center; | |||
transform: translateY(10%); | |||
transform: translateY(100%); |
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.
Probably needs to be adjusted a bit
dont merge this yet need to remove a commit |
This reverts commit dee59e0.
ok should be good merge, nav will be done 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.
Looks good, gonna merge
closing #30 for this