Skip to content
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

Complete SeeMore Project -- NinjaParty #10

Open
wants to merge 163 commits into
base: ninjaparty/master
Choose a base branch
from

Conversation

sallyamoore
Copy link
Contributor

We are in the process of displaying feeds. Right now, the instagram feed displays as a giant hash of recent instagram posts and the twitter feed displays as a giant list of tweets. We haven't combined them, and we have been working in pairs on them. As a result, you'll see in the feeds#index method that there are two chunks of code doing things that potentially conflict. Ignore the feeds#index or look at it as if only one of the if @user &&... blocks exists.

We use the twitter gem (but not the instagram one).

It's going well! :)


Thu Aug 13: Final PR

We finished the project requirements a couple days early and then spent the rest of the time getting tests to pass (using VCR for API calls), some refactoring, and then fixing tons of bugs caused by refactoring. We prevented app users from being able to follow the same social media account twice, disabled dev login on production, and made all instagram and twitter posts links to the actual posts online.

http://ninjaparty.herokuapp.com/

lilagrc added a commit that referenced this pull request Aug 8, 2015
@people.sort_by! { |person| person.username.downcase }

@feed = []
@people.each do |person|
Copy link

Choose a reason for hiding this comment

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

A brief review of your code shows that you're not using this @people variable in the index view (though you are using a @people in the people action). So, first thought is that this does not need to be an instance variable. Second that is that the sort_by! block on line 17 is unnecessary, as you sort again on line 24. The sort on @feed makes sense to me as that's what you're outputting on the page. Sorting can be really expensive, so please take a moment to figure out if it's really necessary.

@jnf
Copy link

jnf commented Aug 10, 2015

Great start here. Please review as a group when you can and send me a slack message after. I'd like to ask a couple of followup questions. Thanks!

@jnf
Copy link

jnf commented Aug 13, 2015

@sallyamoore @elsamoluf @wangg131 @acmei Ok, #21 merged in your first week code, so this PR is just second week now. Please update title and description when you're done with changes.

@acmei acmei changed the title WIP - End of first week PR Complete SeeMore Project -- NinjaParty Aug 14, 2015
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants