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

profile photos as thumbnails #5521

Merged
merged 1 commit into from Feb 14, 2015

Conversation

Projects
None yet
7 participants
@arlogn
Copy link
Contributor

arlogn commented Jan 4, 2015

Proposal. Now that the profile page has been ported to bootstrap why not list the photos as a grid of thumbnails instead of a long list?

profile-photos

@Flaburgan

This comment has been minimized.

Copy link
Member

Flaburgan commented Jan 4, 2015

Duplicate of #5357 I didn't check the code so I don't know which one is the best.

@arlogn

This comment has been minimized.

Copy link
Contributor

arlogn commented Jan 4, 2015

Sorry, didn't see #5357 .

@svbergerem

This comment has been minimized.

Copy link
Member

svbergerem commented Feb 9, 2015

@arlogn I think this is already a step in the right direction. I'd remove the gray and blue backgrounds and would try to improve the delete button which I don't like right now. (too small? depending on the ratio of the images too far away from the image)

@Flaburgan

This comment has been minimized.

Copy link
Member

Flaburgan commented Feb 9, 2015

@svbergerem maybe you want to check #5357 ?

@svbergerem

This comment has been minimized.

Copy link
Member

svbergerem commented Feb 9, 2015

@Flaburgan I said in the comments over there that I helped @StefOfficiel to open a PR and I already checked the code. Like others already said that is plain CSS copied into one of our files instead of changing the existing CSS. StefOfficiel wasn't able to install a pod on his computer because he had no access to a unix based OS. Without a pod where you can test your code it will be hard to integrate some existing code into our codebase.

Codewise this PR is already looking good. However we might want to check if there are some parts in #5357 that we can use here.

@goobertron

This comment has been minimized.

Copy link

goobertron commented Feb 9, 2015

I also think this is a good approach, and I like the thumbnail view. I'm not sure about removing the grey background completely, as it gives the page a more ordered 'grid'-like look. Without a background, those images, which are different sizes and shapes, could look a bit randomly placed on the screen. I think, however, that the current background is too bold, so perhaps something more subtle, either a lower percentage grey, or a light border-shadow or something. I'm not quite sure right now.

If you attempt to delete an image from this page, will it ask whether you want also to delete the post which contains that image (assuming it was uploaded as part of a post)? That would be a useful feature, to avoid unintentional 'orphan' posts with missing images. It could perhaps display the post in a pop-up pane, with 'delete post' and 'delete image only' buttons. Or will the delete feature only be available on old profile photos?

@goobertron

This comment has been minimized.

Copy link

goobertron commented Feb 9, 2015

Oh, sorry, ignore the last part - I've just remembered that we already have image deletion from this page. In which case my question is a separate issue, not for this PR.

@arlogn

This comment has been minimized.

Copy link
Contributor

arlogn commented Feb 9, 2015

@goobertron has explained my intentions. I'm not so convinced but without the background convinces me even less. I dunno, I also tried it with a slight border, but I didn't like.

@svbergerem

This comment has been minimized.

@StefOfficiel

This comment has been minimized.

Copy link

StefOfficiel commented Feb 9, 2015

Hello! At the time of my participation I did not know that D * was undergoing modification to Bootstrap. I think it might be good too to integrate thumbnails Boostrap features that are not too bad. I like the version with the gray background, it distinguishes the images.

@arlogn

This comment has been minimized.

Copy link
Contributor

arlogn commented Feb 10, 2015

@svbergerem Dotted? Looks nice. Very soft, seems like a good alternative to the background. (Strange, when I tried the border I didn't like).

The last image (I suppose hovered) seems to lose the top border. There's a problem with the button?

@svbergerem

This comment has been minimized.

Copy link
Member

svbergerem commented Feb 10, 2015

@arlogn The border is dashed. On the last image you see the hover effect. The control bar has a white background and width: 100% so you can see the buttons on all images with all kinds of colors. This also overlaps the border which might happen because of z-index: 6 on the controls div but I didn't test that. This was just a quick test to give you some input so we can find a good solution we can all (more or less) agree on.

Btw. the block user and report photo icons are missing although this is not my photo stream. We have the same issue on the develop branch. If you want you can also fix that in this PR. The icons are already present in the template file so I guess this should be only a CSS issue. Of course, only if you want to. We can also work on that in another issue.

@arlogn

This comment has been minimized.

Copy link
Contributor

arlogn commented Feb 10, 2015

I did some tests. Controls div 100% with background always overlaps the border because its parent exceeds the size of the thumbnail. Acting on z-index we have a problem with the hover. The controls must be on top and we have to maintain the overflow visible on the parents to show the tooltip.
A size of 15x15 on the top-right is a good solution for me. It has a white background, is in the padding section and should have always a good visibility. What do you think?
delete
thumbs

@goobertron

This comment has been minimized.

Copy link

goobertron commented Feb 10, 2015

I'm not personally that keen on the dashed border: it looks a bit 'tear-off' to me (like perforations on a voucher or postage stamp). I think I prefer a solid, thought faint, border. However, I don't dislike the dashed border nearly enough to oppose it. I think the new design overall is a big improvement.

@arlogn arlogn force-pushed the arlogn:profile-photos-as-thumbnails branch from 60ff994 to 4027bbe Feb 11, 2015

@arlogn

This comment has been minimized.

Copy link
Contributor

arlogn commented Feb 11, 2015

extra-light background (fefefe)
border solid
tiny box shadow

thumbs

doesn't look bad at all.

@goobertron

This comment has been minimized.

Copy link

goobertron commented Feb 11, 2015

Now that's what I'm talking about! Nice design, thanks.

@goobertron

This comment has been minimized.

Copy link

goobertron commented Feb 11, 2015

Travis is failing on the 'And I delete a photo' step of /features/desktop/profile_photos.feature because it can't find the .delete CSS class. If you could fix that test in the feature file to search for whatever is name of the replacement class, that would be great.

@arlogn arlogn force-pushed the arlogn:profile-photos-as-thumbnails branch from a5cb080 to 8f57893 Feb 11, 2015

@arlogn

This comment has been minimized.

Copy link
Contributor

arlogn commented Feb 11, 2015

@goobertron the test is ok for me, I don't understand why it fails on Travis. I have not removed nor renamed the .delete class.

test

@Flaburgan

This comment has been minimized.

Copy link
Member

Flaburgan commented Feb 12, 2015

Not sure about the shadow, the design is quite flat elsewhere, but I guess we can see that when porting to bootstrap 3.

@arlogn

This comment has been minimized.

Copy link
Contributor

arlogn commented Feb 12, 2015

What's the problem? When will be ported to bootstrap 3 we just have to change the grid class.

@Flaburgan

This comment has been minimized.

Copy link
Member

Flaburgan commented Feb 12, 2015

The port to bootstrap 3 will probably come with a new design.

@arlogn

This comment has been minimized.

Copy link
Contributor

arlogn commented Feb 12, 2015

Well, I was hoping to see that in the next 0.5, they are just a few lines of css.

@Flaburgan

This comment has been minimized.

Copy link
Member

Flaburgan commented Feb 12, 2015

Yeah sure! Sorry if I was not clear, my point was, I'm not too sure about the shadow, but we can merge it as it is anyway because everything will probably be redesigned with the port to bootstrap 3.

@goobertron

This comment has been minimized.

Copy link

goobertron commented Feb 12, 2015

I agree: this design is (a) a lot better than the existing page, and (b) fits in well with Diaspora's current UI. When Bootstrap 3 is introduced, the whole site might be redesigned, so at that point this design might be changed.

In the meantime I have pulled your branch and run bin/cucumber features/desktop/profile_photos.feature, and the same delete photo step is failing locally, so I'm not sure why you were able to get it to pass. Perhaps something in your branch or test suite needs updating? I can't answer why that's happening, but hopefully you can work it out and find how to fix it.

# features/step_definitions/profile_steps.rb:9
  Unable to find css ".delete" (Capybara::ElementNotFound)
  ./features/step_definitions/profile_steps.rb:11:in `/^I delete a photo$/'
  -e:1:in `<main>'
  features/desktop/profile_photos.feature:32:in `And I delete a photo'
@arlogn

This comment has been minimized.

Copy link
Contributor

arlogn commented Feb 12, 2015

@Flaburgan OK :)
@goobertron you're right. Now should be fixed.

There's a way to squash unpushed commits with that already pushed before pushing?

@goobertron

This comment has been minimized.

Copy link

goobertron commented Feb 12, 2015

Glad you were able to fix it.

There's a way to squash unpushed commits with that already pushed before pushing?

Yup. Here are instructions @jhass gave me ages ago:

git checkout develop
git pull upstream develop
git checkout branch_name
git rebase -i develop

Just delete the line with the merge commit
Leave 'pick' for your first commit
Select 'pick' or 'squash' (or 'fixup' to omit the commit's message) where appropriate
Save and quit

git push -f origin branch_name

If you get an error:

error: could not apply [commit number]

open the conflicting files in an editor and fix the conflicts, then run

git rebase --continue

until all conflicts are solved, then finally

git push -f origin branch_name

Hope that works for you! And thanks for this PR.

@arlogn

This comment has been minimized.

Copy link
Contributor

arlogn commented Feb 12, 2015

Fails again, I will check back later.
@goobertron I was not clear. This is what I do now, but I have to push all commits. Then rebase, squash and push again. What happens if I try to squash locally an unpushed commit with one already pushed? I have read that in this case I will not be more able to push on the same branch but I have to create a new branch and squash on that. Is this right?

@jaywink

This comment has been minimized.

Copy link
Contributor

jaywink commented Feb 12, 2015

Those tests seem unrelated. If you can just squash the commits in this branch like goob indicated, that would be awesome :) Nice work!

Btw, changelog entry would be nice too.

@svbergerem

This comment has been minimized.

Copy link
Member

svbergerem commented Feb 12, 2015

@arlogn When you change commits you already pushed you will have to use push -f the next time. You can still push them to the same branch.

About your last screenshot: I still don't like the 'delete' button. I think it should have at least some space to the top and to the right. There will be also some other buttons (report and ignore) that are not visible right now. (which is a bug) They will also need some background so I think we should think about something similar to the 'control bar' I had in this screenshot.

But even without that change your PR is still a big improvement for the photos page so if you don't want to do it right now we can still merge your PR and do some refinement later.

@goobertron

This comment has been minimized.

Copy link

goobertron commented Feb 12, 2015

@arlogn you can use the method I outlined locally. The -f in push -f means 'force', so it will accept the push even though you've merged the previously pushed commits with others locally. Honestly, it works - I use it often, as I frequently make mistakes which means multiple commits!

@svbergerem

There will be also some other buttons (report and ignore) that are not visible right now. (which is a bug)

Really? You mean we can ignore and/or report individual photos, as opposed to posts? I didn't know that. In any case, I don't see a conflict here, because the delete button is only for photos on your own profile, so ignore/report wouldn't be displayed on those photos, as they're your own. Unless I've misunderstood you.

I still don't like the 'delete' button. I think it should have at least some space to the top and to the right.

I'd also like a bit more padding between the delete button and the border, ideally, but I'm happy for the PR to be merged as is.

@arlogn

This comment has been minimized.

Copy link
Contributor

arlogn commented Feb 13, 2015

Thanks guys and sorry beacause I can not make myself understood.
I know how to squash commits and normally I proceed as reported by @goobertron.
I try to be clearer.
Suppose there is one pushed commit on this PR and i'm going to fix the test that fails:

  1. I start to work on a new file
  2. Commit the changes
  3. Push the commit to the remote repo
  4. Rebase interactive
  5. Squash the two commits into one
  6. Push again forcing

What i'm looking for is:
there's a way to squash the new commit into that already pushed after the step 2 (before pushing)?
What happens skipping the step 3?

@svbergerem ok, I will make the button slightly larger and more spaced from the border. It has already a white background.

@svbergerem

This comment has been minimized.

Copy link
Member

svbergerem commented Feb 13, 2015

@arlogn You can skip step 3 without any problems. It is even better to skip the third step because Travis runs every time someone pushes changes to a PR. You already know that you want to squash the commits so there is no need to run the tests before squashing.

I know that the button already has a white background. There are some buttons missing (https://github.com/arlogn/diaspora/blob/profile-photos-as-thumbnails/app/assets/templates/photo_tpl.jst.hbs#L6) that are just not shown because of some missing css. When we add the missing css the white background will likely overlap the images which could look weird. As I said before you don't have to fix the missing css for those buttons so your current approach is fine. However when we readd the missing buttons we might have to adjust the background.

The failing tests are unrelated so I'd say just add some additional space to the button, squash the commits and then this PR should be ready to merge.

@goobertron

This comment has been minimized.

Copy link

goobertron commented Feb 13, 2015

As Steffen says, just omit step 3 - merge and squash the commits locally before pushing them, then force-push the resultant commit to your remote branch. It does work, I promise you. Your already-pushed commit(s) will be removed in favour of the new, merged commit.

@arlogn arlogn force-pushed the arlogn:profile-photos-as-thumbnails branch from 89a4143 to 75aec14 Feb 13, 2015

}
}
&:hover > .bd > .controls { background: #fff; }
}

This comment has been minimized.

@houndci-bot

houndci-bot Feb 13, 2015

Rule declaration should be followed by an empty line

#main_stream > div > .photo {
& > .media {
overflow: visible;
> .bd {

This comment has been minimized.

@houndci-bot

houndci-bot Feb 13, 2015

Rule declaration should be preceded by an empty line
Selector should have depth of applicability no greater than 2, but was 5

text-align: center;
line-height: 15px;
}
}

This comment has been minimized.

@houndci-bot

houndci-bot Feb 13, 2015

Rule declaration should be followed by an empty line

> .bd {
position: relative;
overflow: inherit;
> .controls {

This comment has been minimized.

@houndci-bot

houndci-bot Feb 13, 2015

Rule declaration should be preceded by an empty line
Selector should have depth of applicability no greater than 2, but was 6

border: 1px solid $border-grey;
background: #fefefe;
box-shadow: 3px 3px 2px #eee;
img {

This comment has been minimized.

@houndci-bot

houndci-bot Feb 13, 2015

Rule declaration should be preceded by an empty line
Selector should have depth of applicability no greater than 2, but was 5

@@ -12,7 +9,42 @@
}
}

#main_stream > div > .photo {

This comment has been minimized.

@houndci-bot

houndci-bot Feb 13, 2015

Avoid using id selectors
Selector should have depth of applicability no greater than 2, but was 3
Selector main_stream should be written in lowercase with hyphens

& > .media {
overflow: visible;
> .bd {
position: relative;

This comment has been minimized.

@houndci-bot

houndci-bot Feb 13, 2015

Properties should be ordered overflow, position

position: relative;
overflow: inherit;
> .controls {
position: absolute;

This comment has been minimized.

@houndci-bot

houndci-bot Feb 13, 2015

Properties should be ordered height, line-height, position, right, text-align, top, width

&:hover > .bd > .controls { background: #fff; }
}
.thumbnail {
height: 200px;

This comment has been minimized.

@houndci-bot

houndci-bot Feb 13, 2015

Properties should be ordered background, border, box-shadow, height, line-height, margin, padding, text-align

@@ -12,7 +9,42 @@
}
}

#main_stream > div > .photo {
& > .media {

This comment has been minimized.

@houndci-bot

houndci-bot Feb 13, 2015

Selector should have depth of applicability no greater than 2, but was 4
Unnecessary parent selector (&)

@svbergerem

This comment has been minimized.

Copy link
Member

svbergerem commented Feb 13, 2015

@arlogn Ignore those comments. ;) We don't have a styleguide for SCSS yet so I'll turn this off.

@arlogn

This comment has been minimized.

Copy link
Contributor

arlogn commented Feb 13, 2015

@svbergerem @goobertron thanks for your help, prevent Travis to run the tests twice on the same code was exactly my intention.

I tried to slightly increase the space of the button from the border but it overlaps to the images that fill all the space available. So I gave up, later we can find a better option.

@@ -8,6 +8,6 @@

When(/^I delete a photo$/) do
find('.photo.loaded').hover
find('.delete', :match => :first).click
find('.delete', :visible => false, :match => :first).click

This comment has been minimized.

@svbergerem

svbergerem Feb 13, 2015

Member

The button has to be visible for the user so looking for an invisible button doesn't look like the right thing to do.

Here is another solution which fixed the test for me:

diff --git a/features/step_definitions/profile_steps.rb b/features/step_definitions/profile_steps.rb
index 70f9acc..60fd76f 100644
--- a/features/step_definitions/profile_steps.rb
+++ b/features/step_definitions/profile_steps.rb
@@ -7,7 +7,7 @@ And /^I mark myself as safe for work$/ do
 end

 When(/^I delete a photo$/) do
-  find('.photo.loaded').hover
-  find('.delete', :visible => false, :match => :first).click
+  find('.photo.loaded .thumbnail', :match => :first).hover
+  find('.delete', :match => :first).click
 end

This comment has been minimized.

@arlogn

arlogn Feb 14, 2015

Contributor

sure, that way is much better.

@arlogn arlogn force-pushed the arlogn:profile-photos-as-thumbnails branch from 75aec14 to 43a2a5c Feb 14, 2015

@svbergerem svbergerem added this to the next-major milestone Feb 14, 2015

@svbergerem svbergerem merged commit 43a2a5c into diaspora:develop Feb 14, 2015

2 checks passed

continuous-integration/travis-ci The Travis CI build passed
Details
hound Hound has reviewed the changes.

svbergerem pushed a commit that referenced this pull request Feb 14, 2015

Steffen van Bergerem
@svbergerem

This comment has been minimized.

Copy link
Member

svbergerem commented Feb 14, 2015

Thank you! :)

@Flaburgan

This comment has been minimized.

Copy link
Member

Flaburgan commented Feb 14, 2015

Awesome! Let's pull this in diaspora-fr :D

@goobertron

This comment has been minimized.

Copy link

goobertron commented Feb 14, 2015

Thanks for this, @arlogn!

@StefOfficiel

This comment has been minimized.

Copy link

StefOfficiel commented Feb 15, 2015

Hello !
Allow me to make a comeback after testing on D-fr. This thumbnail is not ergonomic.
You must click on the photo to be big. While the purpose of a thumbnail is to click anywhere on the thumbnail to view the photo in full screen.
Thx !

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