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

Feature/4347 add photo link to leftsidebar #4369

Merged
merged 9 commits into from Aug 19, 2013

Conversation

Projects
None yet
7 participants
@juliaguar
Contributor

juliaguar commented Aug 8, 2013

solving issue #4347 by creating a link, in the left side menu, to the photos uploaded by a user in their user profile,
and below the link to photos show the thumbnails of the last 3 photos.
We also renamed contact_count to item_count, because now it is also used to show the total number of photos.

@@ -2467,9 +2480,7 @@ ul.left_nav
> a:not(.sub_selected)
:color #333
.contact_count
:font

This comment has been minimized.

@svbergerem

svbergerem Aug 8, 2013

Member

why did you remove those 2 lines? Are they not necessary anymore?

This comment has been minimized.

@juliaguar

juliaguar Aug 8, 2013

Contributor

because I moved it up to line 2439 (https://github.com/diaspora/diaspora/pull/4369/files#L1R2439) and now it should inherit from there.

This comment has been minimized.

@svbergerem
@svbergerem

This comment has been minimized.

Member

svbergerem commented Aug 8, 2013

As I opened a similar PR which adds the contact list to the profile I think we should try to unify the look.

I reused the css-id "contacts_of_contact" which sets the size of an avatar to 45x45 px, centers the "see all"-link and adds a bottom-margin to .section. Either you use the same style as declared in #contacts_of_contact or I will change the style in my PR.

Another thing we should talk about is text used for the "see all"-link. You use layouts.header.view_all while I use people.show.see_all which was used before for the contact list.

@Flaburgan

This comment has been minimized.

Member

Flaburgan commented Aug 8, 2013

What about merge the two PRs and then create a new one to unify the design? Looks easier to me than trying to work on two separate PRs at the same time.

@svbergerem

This comment has been minimized.

Member

svbergerem commented Aug 8, 2013

Sounds good. We just have to be careful with the refactoring of .contact_count as this is done in both PRs. (and much better done in this PR)

Edit: I copied the changed files from this PR so the refactoring shouldn't be a problem

@Flaburgan

This comment has been minimized.

Member

Flaburgan commented Aug 14, 2013

Is this ready to be merged or are you still working on something?

@DeadSuperHero

This comment has been minimized.

Member

DeadSuperHero commented Aug 18, 2013

@carolinagc @juliaguar What's the status on this? :) AFAIK, #4360 is somewhat dependant on this PR, if we want to go for unifying how both of them work.

@svbergerem

This comment has been minimized.

Member

svbergerem commented Aug 18, 2013

@DeadSuperHero AFAIK #4360 is not really dependant but after merging BOTH PRs it would make sense to unify the look.

@carolinagc

This comment has been minimized.

Contributor

carolinagc commented Aug 18, 2013

Well, we are done with it for the moment, except if there are any other comments :)

@jhass

View changes

app/views/people/_profile_sidebar.html.haml Outdated
@@ -43,6 +44,17 @@
%h4
=t('.born')
= birthday_format(person.birthday)
%li
- unless @photos.blank?

This comment has been minimized.

@jhass

jhass Aug 18, 2013

Member

- if @photos.present? - unless is better than if ! but if is better than unless ;)

This comment has been minimized.

@carolinagc

carolinagc Aug 18, 2013

Contributor

ok :-) .. we were following the pattern of the rest of the file O:)

@jhass

View changes

app/views/people/_profile_sidebar.html.haml Outdated
.item_count
= "#{@photos.count}"
- @thumbs = @photos.limit(3)
- for photo in @thumbs

This comment has been minimized.

@jhass

jhass Aug 18, 2013

Member

@photos.limit(3).each do |photo| - no need for another instance or even local variable, never create them in a view anyway. Also avoid for in Ruby unless you're sure you want its scoping behavior (which is unexpected if you're coming from other languages).

@jhass

View changes

features/profile_photos.feature Outdated
And I press "Share"
Scenario: see my own photos
And I am on "robert@grimm.grimm"'s page

This comment has been minimized.

@jhass

jhass Aug 18, 2013

Member

Always start a scenario with When

@jhass

View changes

features/profile_photos.feature Outdated
Scenario: see my own photos
And I am on "robert@grimm.grimm"'s page
When I follow "View all"

This comment has been minimized.

@jhass

jhass Aug 18, 2013

Member

And use And if there's a step before it that wasn't an expectation.

@jhass

View changes

features/profile_photos.feature Outdated
Scenario: see my own photos
And I am on "robert@grimm.grimm"'s page
When I follow "View all"
Then I should be on person_photos page

This comment has been minimized.

@jhass

jhass Aug 18, 2013

Member

I'd vote for Then I should be on my photos page

@jhass

This comment has been minimized.

Member

jhass commented Aug 18, 2013

Tbh. I completely lost track with all the PRs modifying the left sidebar. If you all could make me a list of what you'd like merged in what order I'd highly appreciate that.

@svbergerem

This comment has been minimized.

Member

svbergerem commented Aug 18, 2013

I'll propose a list and hope everyone is happy with that one. If not just tell me:

#4369 and #4360 (order doesn't matter), then someone has to work on unifying the looks
#4304, then #4374

@DeadSuperHero

This comment has been minimized.

Member

DeadSuperHero commented Aug 18, 2013

Sounds like a plan to me!

@jhass

This comment has been minimized.

Member

jhass commented Aug 18, 2013

Also we need a rebase against develop so that Travis runs the tests. Watch out for conflicts!

@goobertron

This comment has been minimized.

goobertron commented Aug 19, 2013

Not sure if it matters in code, but in English should that be "Then I should be on my photos page" (not "by")?

@jhass

This comment has been minimized.

Member

jhass commented Aug 19, 2013

Uh, right, typo on my side.

@jhass

This comment has been minimized.

Member

jhass commented Aug 19, 2013

Alright, thank you!

jhass added a commit that referenced this pull request Aug 19, 2013

Merge pull request #4369 from Team-D/feature/4347-add_photo_link_to_l…
…eftsidebar

Feature/4347 add photo link to leftsidebar

@jhass jhass merged commit c0b86dc into diaspora:develop Aug 19, 2013

1 check passed

default The Travis CI build passed
Details

@juliaguar juliaguar deleted the Team-D:feature/4347-add_photo_link_to_leftsidebar branch Aug 19, 2013

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment