Skip to content
This repository has been archived by the owner on Jan 15, 2019. It is now read-only.

Project Listing Page #35

Merged
merged 10 commits into from
Mar 17, 2018
Merged

Project Listing Page #35

merged 10 commits into from
Mar 17, 2018

Conversation

sumnerevans
Copy link
Member

@sumnerevans sumnerevans commented Dec 18, 2017

Resolves #13

Features

  • New "Projects" page listing all of the projects and viewing information about them.
  • Projects can be added from the admin panel
  • Projects can have the following properties:
    • Name (required)
    • Description
    • Website
    • Repository
    • Video
    • Image
    • Team Members (list of users)

Questions

  • Currently, the image can be any shape and can be transparent. Unlike the Contact page, where the pictures have backgrounds, there is very little contrast between project images and the surrounding page. Any ideas on how to improve that?
  • What do you think of the overall design? I used a lot of ideas from ACM Website Redesign Proposal #15.

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:
2017-12-18-17 14 53

Projects page on a medium-sized screen:
2017-12-18-17 13 39

Projects page on a narrow screen:
2017-12-18-17 12 06

-webkit-border-radius: 4px;
-moz-border-radius: 4px;
border-radius: 4px;
color: #c09853;
Copy link
Contributor

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.

Copy link
Member Author

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.)

Copy link
Contributor

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.

<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" />
Copy link
Contributor

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.

@@ -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)
Copy link
Contributor

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):
Copy link
Contributor

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...

Copy link
Member Author

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.

@jackrosenthal
Copy link
Contributor

Currently, the image can be any shape and can be transparent. Unlike the Contact page, where the pictures have backgrounds, there is very little contrast between project images and the surrounding page. Any ideas on how to improve that?

Look at the LUG presentations page... what do you think of how that one is implemented?

@sumnerevans
Copy link
Member Author

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.)

@sumnerevans
Copy link
Member Author

@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()
Copy link
Contributor

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?

Copy link
Member Author

Choose a reason for hiding this comment

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

This PR doesn't deal with individual project pages, so I think this is a consideration for #40. Personally, I don't like the short URLs. I think they are ugly. I've added this to the Considerations section on #40.

Copy link
Contributor

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.

Copy link
Member Author

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 id project_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

Copy link
Contributor

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.

samsartor
samsartor previously approved these changes Jan 17, 2018
Copy link
Contributor

@samsartor samsartor left a comment

Choose a reason for hiding this comment

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

Looks nice!

@jackrosenthal
Copy link
Contributor

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.

@sumnerevans
Copy link
Member Author

@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.

@sumnerevans
Copy link
Member Author

@samsartor , on your plate for review.

Copy link
Contributor

@samsartor samsartor left a comment

Choose a reason for hiding this comment

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

Looks good!

@samsartor samsartor merged commit 5d8bab2 into develop Mar 17, 2018
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants