-
Notifications
You must be signed in to change notification settings - Fork 3
Conversation
acmwebsite/public/css/style.css
Outdated
-webkit-border-radius: 4px; | ||
-moz-border-radius: 4px; | ||
border-radius: 4px; | ||
color: #c09853; |
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.
Indentation in CSS should use 2 spaces. Please don't change 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.
We've been very inconsistent with that throughout the CSS file so I was confused. I chose 4 because it seemed to be more common. (I actually prefer 2, so I'm happy to change 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.
Added note to contributing guidelines.
acmwebsite/templates/projects.xhtml
Outdated
<b>Contributors:</b> | ||
<ul> | ||
<li py:for="tm in project.team_members"> | ||
<a href="${tg.url('/u/%s' % tm.user_name)}" py:content="tm.display_name" /> |
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 use printf
style formatting. The str.format
function available since Python 2.5 is a better choice.
acmwebsite/lib/helpers.py
Outdated
@@ -29,6 +29,10 @@ def icon(icon_name): | |||
return Markup('<i class="glyphicon glyphicon-%s"></i>' % icon_name) | |||
|
|||
|
|||
def fa_icon(icon_name): | |||
return Markup('<i class="fa fa-%s"></i>' % icon_name) |
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 use printf
style formatting. The str.format
function available since Python 2.5 is a better choice.
from acmwebsite.model import DBSession, Project | ||
|
||
|
||
class ProjectsController(BaseController): |
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.
Will there ever be the possibility of having a page for separate projects? If yes, the OO style thing we did with the meetings/surverys might be a good choice.
Not priority... something we eventually might consider. Can be done way after this has been merged...
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.
Yes. That's why I had this as a separate controller. It's kinda dumb right now, but in the future, I can definitely see eventually having a page for each project (for example, it may include more details about the project and/or additional media such as screenshots). I think is should be pretty easy to add on a ProjectController
for the individual projects with this setup.
Look at the LUG presentations page... what do you think of how that one is implemented? |
@jackrosenthal , I took some inspiration from the presentations page and updated the design here. See the new screenshots in the issue description at the top. (I left links to the old ones for reference.) |
@jackrosenthal , I don't think there should be anything major holding this up. Could you please re-review this PR? |
@@ -49,6 +50,7 @@ class RootController(BaseController): | |||
schedule = ScheduleController() | |||
error = ErrorController() | |||
contact = ContactController() | |||
projects = ProjectsController() |
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.
Individual projects should probably be located at acm.mines.edu/p/###
(users are /u/***
, surveys are /s/###
, etc). Is there a way to do that while keeping the /projects
list page? Adding an additional ProjectController
?
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.
Let's continue use of short URLs even if Sumner does not like it... better for Emails, and looks better too.
Sorry Sumner.
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.
@jackrosenthal, fine by me. Still doesn't matter here, this PR only deals with the project listing page. From what I can see, we agree on this URL scheme:
/projects
- shows a list of all the projects with links to the individual projects/p/<project_id>
- shows the project with idproject_id
I think the real question is how to implement this. I think worst case scenario we have to add a ProjectLookupController
and ProjectController
in addition to the ProjectsController
. Alternatively, we could expose the /projects
endpoint on the root controller which would avoid that mess. My only concern with that is that it clutters up the root controller.
CC: @samsartor
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 was mainly thinking about future work on #40. Maybe we should eventually rename ProjectsController
to ProjectListController
so that it does not get confused with the incoming ProjectController
and Project[s|Lookup]Controller
. I'm feeling confused just listing them.
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 nice!
use /p/unique_textual_project_name instead of an ID. The ID makes it hard to remember or read (which destroys the whole point of the short url. |
@jackrosenthal , again, that's not applicable to this PR since there are no individual pages for projects as of this PR. Any feedback on individual pages should be on #40. |
3551c8c
to
23071bf
Compare
@samsartor , on your plate for review. |
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!
Resolves #13
Features
Questions
Screenshots
Old (for reference)
Projects page on desktop:
2017-12-18-00 36 36
2017-12-18-00 55 02
Projects page on small screen:
2017-12-18-00 49 52
New
Projects page on large screen:
Projects page on a medium-sized screen:
Projects page on a narrow screen: