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

Redesign profile page and port to Bootstrap #4657

Merged
merged 1 commit into from Oct 6, 2014

Conversation

Projects
None yet
@svbergerem
Copy link
Member

svbergerem commented Jan 2, 2014

to do:

  • profile sidebar
  • mention facebox
  • /people/guid/contacts
  • adapt tests

This PR fixes

people view

@goobertron

This comment has been minimized.

Copy link

goobertron commented Jan 9, 2014

Thanks for taking all these tasks on, Steffen. It's really valuable work.

@@ -4,6 +4,9 @@

class PeopleController < ApplicationController
before_filter :authenticate_user!, :except => [:show, :last_post]

layout ->(c) { request.format == :mobile ? "application" : "with_header_with_footer" }

This comment has been minimized.

@jhass

jhass Feb 9, 2014

Member

You don't need to copy that line from ApplicationController, it's inherited ;)

@@ -0,0 +1,6 @@
- unless person == current_user.person
- contact = current_user.contacts.find_by_person_id(person.id)
- contact ||= Contact.new(:person => person)

This comment has been minimized.

@jhass

jhass Feb 9, 2014

Member

That looks like it wants to be a helper.

@jhass

This comment has been minimized.

Copy link
Member

jhass commented Feb 9, 2014

I think I'd like to see all those big if bootstrap else blueprint to be extracted into something like _foo_bootstrap.haml / _foo_blueprint.haml and then just something like = render_bootrstrap_or_blueprint 'foo' with

def render_bootstrap_or_blueprint template, *args
  template << bootstrap ? '_bootstrap' : '_blueprint'
  render template, *args
end

(More pseudocode, but I think it demonstrates the idea)

@svbergerem

This comment has been minimized.

Copy link
Member

svbergerem commented Feb 11, 2014

I'll finish #4678 first and then work on hovercards. Afterwards I'll come back to this PR, work on the suggestions and see what else needs to be done.

@svbergerem

This comment has been minimized.

Copy link
Member

svbergerem commented Mar 29, 2014

a small preview:

@jhass

This comment has been minimized.

Copy link
Member

jhass commented Mar 29, 2014

I don't want to belittle your work, but man, that are heavy design changes. Not that I think they're bad, but that's not porting.

These really shouldn't be mixed into this task, not that heavy changes. The SPV is a good example on how to use Bootstrap while keeping the general look & feel of diaspora, but we do need throughout discussion before fundamentally changing the look & feel, I'm sorry.

@svbergerem

This comment has been minimized.

Copy link
Member

svbergerem commented Mar 29, 2014

@MrZyx Sure, I'll do less design changes in this PR and open a new one afterwards which will add those design changes.

How about this:

There is still a lot of work to do but I just want to know if those changes are "too heavy".

@jhass

This comment has been minimized.

Copy link
Member

jhass commented Mar 29, 2014

That's going into a defensible direction, yes. Thank you.

I'm not too sure if making contacts an item on the same level as stream and photos is a good idea, I'd expect from the previous two that it would too change the right side, which it probably doesn't.

@goobertron

This comment has been minimized.

Copy link

goobertron commented Mar 29, 2014

Hmm, I quite like your design ideas. Let's discuss them separately, as MrZYX says.

Re the latest screenshot:

  • I notice that you've moved the screen name and Diaspora ID to the LH column. This kind of makes sense. However moving profile tags there as well means that some items won't be visible on the screen without scrolling when the user has a bio. This might be a drawback.
  • Have we lost the previews of 8 uploaded photos and 8 avatars? I rather liked them and would be sad to see them go. I'm open to a new approach, however, and making the column 'cleaner' as you're trying to do is no bad thing. One suggestion: show the 8 image previews on hover. Any good?
  • When you have some profile tags, will they be displayed as a list in the LH column, as they are currently at the top of the stream? The reason I ask is because you've added a link to the Stream in the LH column, and I don't think it fits there. It gives the impression that the Tags item will filter the stream by tag, as it does in the main stream view. The items in the LH column on the people view are profile items, and there are already two ways to return to the Stream in the header. I think adding it to the LH column will confuse people as to what the other items do.
  • Personally I quite like the name and ID at the top of the stream, because it distinguishes the people view from the other streams. Underneath the avatar, it isn't very prominent. I understand, however, that having this information at the top of the stream does kind of 'break' the consistency of design a bit. I'm fine with this information moving from the top of the stream if it can retain a similar level of prominence.
  • If there's going to be a background colour on hover of LH column items, it should, I think, match the pale blue used for LH items in the main stream view - for now, at least. Then, when everything has been ported to Bootstrap, it can be changed everywhere if it's not a good colour. Then at least there's consistency throughout the app.

I'd actually be fine with dropping the list of tags altogether and dropping the (small) distinction between profile tags and the other tags one follows, but that's another discussion...

Do we actually need the publisher on the people view? I guess it's good to give people the option to start a conversation wherever they are, but you can't post from other people's profile page (obviously), so perhaps there's a case for not doing it from your own. It would remove one item from the top of the page, and make this view less like the main stream (if we no longer have profile information at the top of the stream). But again, another conversation...

@Flaburgan

This comment has been minimized.

Copy link
Member

Flaburgan commented Mar 30, 2014

Do we actually need the publisher on the people view?

I guess the publisher is here because it's his profile, it's not here when you are on someone else profile.

I agree with Goob here, to have the username, the diaspora ID and the tags at the top of the page makes sense to me. The user has to know on which user page he is immediately (the avatar is not enough, we are many to have the Firefox OS avatar for example) and the tags in the left column could be confusing with the stream page, as said previously.

@svbergerem

This comment has been minimized.

Copy link
Member

svbergerem commented Mar 30, 2014

@goobertron @Flaburgan Thank you for your feedback. I moved the username and tags back to the top of the page.

"Stream" is a link to the users profile page. See #4722 and #4745. Maybe one should rename it to "Profile"?

@jhass

This comment has been minimized.

Copy link
Member

jhass commented Mar 30, 2014

Or just "Posts". I think I still don't like the contacts link looking like it would change the right side.

@goobertron

This comment has been minimized.

Copy link

goobertron commented Mar 31, 2014

Ah, right. Perhaps I misunderstood what you're doing here.

Do the three links – Stream, Photos and Contacts – actually show you the person's posts, photos and (visible) contacts in the main part of the page? I.e., they are navigation tools; when clicked, the LH column will remain the same while the RH column changes?

If so, that sounds like a really good idea. (And I agree with MrZYX that in that case 'Posts' would be most appropriate.) However, that's a lot more changes than mere porting. I like the idea, but we'd need to separate the new ideas from the porting, I think.

@goobertron

This comment has been minimized.

Copy link

goobertron commented Mar 31, 2014

Although, thinking about it, as there are links in the current version for photos and contacts, the functionality is not changed, just the design, so perhaps it doesn't need separating. However, as I think MrZYX is saying, currently the contacts link takes you to a completely separate page. Perhaps we need to think about ways to integrate contacts into the profile page in the way that posts and photos are. However, that is work for another PR.

I think there are some cases when displaying someone else's profile page that there will be contacts of theirs which are visible to you (if you are in an aspect which has 'make contacts in this aspect visible to each other). In this case, it could be appropriate to display the contacts link; otherwise, 'Contacts' should be absent, or greyed out. The same with photos: if the person hasn't uploaded any photos, or none which are visible to you, 'Photos' should be absent or greyed out.

Am I understanding your work and your intentions correctly?

By the way, I don't think that the ID and tags necessarily have to be at the top of the page. The screen name, larger and perhaps centred, could be enough. That's perhaps a later refinement, however; as long as it's instantly clear to anyone who lands on the page that they're on a profile page rather than the main stream, that's fine.

@svbergerem

This comment has been minimized.

Copy link
Member

svbergerem commented Mar 31, 2014

However, as I think MrZYX is saying, currently the contacts link takes you to a completely separate page. Perhaps we need to think about ways to integrate contacts into the profile page in the way that posts and photos are. However, that is work for another PR.

That is only right for your own contacts. When you are on another user's profile page the "contacts" link works exactly like you a describing it: It shows you the contacts of that user just like his/hers posts or photos.

I will only show a link to the contacts or photos page if there are any contacts or photos visible to you. You will see that as soon as I push my changes to the branch.

@Flaburgan

This comment has been minimized.

Copy link
Member

Flaburgan commented Mar 31, 2014

@svbergerem I think keeping the name + diaspora ID and tags on the right column before the publisher as it is now is nice, any argument for your change?

@svbergerem

This comment has been minimized.

Copy link
Member

svbergerem commented Mar 31, 2014

@Flaburgan I moved the Message/Mention/Ignore buttons to the top of the page next to the aspect membership dropdown because all those buttons perform some interaction with the user.

When moving the name and diaspora id back to the right column there will be some style problems on smaller displays:

width: 1024px
profile_header_1024

width: 1280px
profile_header_1280

Additionally, the name and diaspora id belong to the posts as well as to the profile information.

@Flaburgan

This comment has been minimized.

Copy link
Member

Flaburgan commented Mar 31, 2014

I'm in favour of keeping the icons instead of the text, how does it look if you replace the text in the buttons by the actual icons?

@svbergerem svbergerem changed the title [WIP] Port publisher and people view to bootstrap [WIP] Port people view to Bootstrap Mar 31, 2014

@goobertron

This comment has been minimized.

Copy link

goobertron commented Mar 31, 2014

Hmm, does this mean we'd be losing the wonderful profile sidebar you designed last year? I'd be sad to see that go, as I think it's a fantastic piece of design.

I'd be more inclined to move the aspects selector button to the LH column than move the other interaction buttons to the top right. I also think we could potentially have the ID and tags in the LH column and just have the screen name as a page 'title', as I suggested before. However, if we do move all the interaction icons to the LH column, there will be more things in the LH column and more space at the top, in which case it probably would make sense to put screen name, ID and tags in the title bar, as in your recent screenshots.

I can see what you're getting at, and it is certainly logical to have all the interaction buttons in the same place. But it would be a shame to lose the wonderful profile side-bar icons as part of this.

@Flaburgan

This comment has been minimized.

Copy link
Member

Flaburgan commented Mar 31, 2014

IMO the profile page as it is right now in the stable branch is pretty nice, I'm not sure to see why we need to change it. The switch between posts, pictures and contacts pages is nice though.

@svbergerem

This comment has been minimized.

Copy link
Member

svbergerem commented Mar 31, 2014

@Flaburgan The main purpose of this PR is to create a responsive profile page with Bootstrap. When displaying that page with a width lower than 768px the whole page will be in one column. IMO the first thing you should see is the users name and the avatar. Then there should be the profile information / the interaction buttons and then the user's posts. When you look at the current view in the stable branch and think of a Bootstrap port the first thing you will see is the avatar, then there are the interaction buttons and the profile information, then the users name the user's posts. That's the reason why I would like to reorder some elements.

IMO it is a good idea to put elements with a similar function next to each other. That's why I would like to have the profile information next to the name and the interaction buttons next to the aspect membership dropdown. I also think that the tags belong to the profile information instead of the username. You can see some of those ideas in the screenshot I already posted.

I think the profile information should be in the LH column. Having the interaction buttons + the aspect membership dropdown also in the LH column will bloat that column.

There are also some elements in the current view that are not that easy to port and make them responsible. Those are especially the interaction buttons and the grids of contacts / photos. Just changing the width from "px" to "%" won't make them look good for all display sizes.

@Flaburgan

This comment has been minimized.

Copy link
Member

Flaburgan commented Apr 2, 2014

I'm sorry but we don't care about responsive, that's why we have a separate mobile version. Or did I miss something?

@svbergerem

This comment has been minimized.

Copy link
Member

svbergerem commented Apr 2, 2014

https://www.loomio.org/d/4vG7NMBS/restructure-ui-code
Thread description, CSS, first point:

Make Layout responsive

@jhass

This comment has been minimized.

Copy link
Member

jhass commented Apr 2, 2014

I'm happy with adaptive as a first step though.

@svbergerem

This comment has been minimized.

Copy link
Member

svbergerem commented Oct 1, 2014

Rebased

@Flaburgan

This comment has been minimized.

Copy link
Member

Flaburgan commented Oct 1, 2014

Brace yourself, merge is coming! /me is excited

@Raven24

This comment has been minimized.

Copy link
Member

Raven24 commented Oct 1, 2014

I'll look over the code one last time tonight (CEST)

@roymckenzie

This comment has been minimized.

Copy link
Contributor

roymckenzie commented Oct 5, 2014

New profile page looks great! Is there a reason why the width of the content area of the nav bar and the width of the profile are going to be inconsistent? Is there plans to increase the width of the content area for the nav bar or tighten up the profile content area? This is glaring to me and inconsistent throughout the site currently. This should be addressed in my opinion. What are your guys' thoughts?

@svbergerem

This comment has been minimized.

Copy link
Member

svbergerem commented Oct 5, 2014

@roymckenzie We have the same problem on most bootstrap pages. I think we should rewrite the navbar using bootstrap elements and entypo icons. This would even make the navbar responsive (and could fix #1960, #4588, #4721 and #5283). First we need to port all pages to bootstrap because they all use the header.

@roymckenzie

This comment has been minimized.

Copy link
Contributor

roymckenzie commented Oct 5, 2014

@svbergerem Cool I am happy to work on the porting of the navigation to use bootstrap and entypo once all the pages are ported. Do we know when that is going to happen? Do we have a list of pages left? I don't mind working to convert a page either. Also, I noticed there is no "satellite" icon for Notifications badge on entypo so the team would need to decide on which icon to replace it with. I know @jhass has mentioned in the past he didn't want to change UI elements in order to just use a new technology, I can respect that.

@svbergerem

This comment has been minimized.

Copy link
Member

svbergerem commented Oct 5, 2014

@roymckenzie the list you mentioned: #4466

@roymckenzie

This comment has been minimized.

Copy link
Contributor

roymckenzie commented Oct 5, 2014

thanks @svbergerem!

</li>
{{/if}}
{{#if show_contacts}}
<li {{#isCurrentPage 'person_contacts' guid}} class="active" {{/isCurrentPage}}>

This comment has been minimized.

@Raven24

Raven24 Oct 6, 2014

Member

the isCurrentPage and isCurrentProfilePage is almost too much logic for a template in my taste.
I'd rather you put that logic in the backbone view and just add three entries to the presenter (e.g. 'class_profile_active'. 'class_photos_active', 'class_contacts_active') that have the string "active" according to the condition.
then you'd use <li class="{{class_profile_active}}"> in the template instead.

This comment has been minimized.

@svbergerem

svbergerem Oct 6, 2014

Member

Sounds good, I'll do that in another PR.

@Raven24

This comment has been minimized.

Copy link
Member

Raven24 commented Oct 6, 2014

alright I added one more comment. It's not a blocker for this PR, but I'd love to see a follow-up addressing that.
Otherwise this is good to go from my side :D

Raven24 added a commit that referenced this pull request Oct 6, 2014

Merge pull request #4657 from svbergerem/bootstrap-people-view
Redesign profile page and port to Bootstrap

@Raven24 Raven24 merged commit 4f87a47 into diaspora:develop Oct 6, 2014

1 check passed

continuous-integration/travis-ci The Travis CI build passed
Details
@Raven24

This comment has been minimized.

Copy link
Member

Raven24 commented Oct 6, 2014

merged!
big thanks to everyone involved ;)

@Flaburgan

This comment has been minimized.

Copy link
Member

Flaburgan commented Oct 6, 2014

Wow! What a PR! Many thanks to everyone, especially Steffen ;)

Didn't spot any bug at the moment (except maybe a border-bottom under each image, so two borders on the photos page), so I'm going to push that on diaspora-fr, let's see what the users think!

@Flaburgan

This comment has been minimized.

Copy link
Member

Flaburgan commented Oct 6, 2014

Oh, there is still #5045 too...

@svbergerem svbergerem deleted the svbergerem:bootstrap-people-view branch Oct 6, 2014

@svbergerem

This comment has been minimized.

Copy link
Member

svbergerem commented Oct 6, 2014

@Raven24 @jhass Thanks you for reviewing this PR. Your comments helped a lot!

@Flaburgan Well spotted, I didn't recognize the second border before. There is also #5291 which is a regression caused by this PR. I'll try to fix both tomorrow.

@pablocubico

This comment has been minimized.

Copy link
Contributor

pablocubico commented Oct 6, 2014

Awesome work! All-bootstrap diaspora is closer than ever!

On Mon, Oct 6, 2014 at 2:16 PM, Steffen van Bergerem <
notifications@github.com> wrote:

@Raven24 https://github.com/Raven24 @jhass https://github.com/jhass
Thanks you for reviewing this PR. Your comments helped a lot!

@Flaburgan https://github.com/Flaburgan Well spotted, I didn't
recognize the second border before. There is also #5291
#5291 which is a regression
caused by this PR. I'll try to fix both tomorrow.


Reply to this email directly or view it on GitHub
#4657 (comment).

@Flaburgan

This comment has been minimized.

Copy link
Member

Flaburgan commented Oct 6, 2014

@svbergerem you can also add to the list a weird one: the number of photo (displayed in the tab selector) is at maximum 15, no matter how many you have.

@svbergerem

This comment has been minimized.

Copy link
Member

svbergerem commented Oct 6, 2014

@Flaburgan looks like we have the same issue on the master branch so that is no regression introduced by this PR. Would you mind opening a new issue? I'll first try to fix regressions caused by this PR.

@goobertron

This comment has been minimized.

Copy link

goobertron commented Oct 6, 2014

Thanks a lot to @svbergerem and the reviewers for all this work.

There's one thing about the new profile which I think is a bug: under the contacts tab, it appears to display all that person's contacts – possibly only if (a) they are on the same pod or (b) they are on a pod running the new profile code (I can't quite work it out).

I think it should only display contacts in the same aspect as you if the person whose profile you are viewing checked 'make contacts in this aspect visible to each other'. I believe this is the behaviour of the current (old) profile page, and think it's important for privacy in Diaspora.

@goobertron

This comment has been minimized.

Copy link

goobertron commented Oct 6, 2014

(Can't edit the post, dammit)

I think you're fixing a few things in a new PR. Perhaps you could look into this while you're doing that. However, if you'd like me to open a new issue for this, let me know.

@svbergerem

This comment has been minimized.

Copy link
Member

svbergerem commented Oct 6, 2014

@goobertron You said the behavior changed with this PR so there is no need to open a new issue. I'll also add it to my list. I don't know how I could have changed that behavior but I'll double check that and see if we need an rspec test for that specific issue.

@goobertron

This comment has been minimized.

Copy link

goobertron commented Oct 6, 2014

I may have mistaken what is going on, but using my account on diaspora-fr, for those of my contacts who are also on diaspora-fr, they show many many contacts under the tab, whereas looking at those same contacts from my account on orkz (running 0.4.1.0), contacts are not shown. Even an account I checked of someone on diaspora-fr with whom I'm not sharing showed me their contacts (some, at least - can't know if it's all).

@svbergerem

This comment has been minimized.

Copy link
Member

svbergerem commented Oct 7, 2014

@goobertron Aspects aren't federated so you can only see your contact's contacts if you are both on the same pod. We have some rspec tests to make sure that you are only allowed to see contacts who are in the same aspect with a visible contact list. It would be great if you could verify that you are able to see contacts you are not allowed to see. (Then we would really have a privacy issue)

@goobertron

This comment has been minimized.

Copy link

goobertron commented Oct 8, 2014

I don't have a way of testing whether I'm shown contacts I shouldn't be. If the code says it only shows contacts I should be seeing, I'm happy to trust that. If I find out later that I am seeing contacts I shouldn't be, I'll open a fresh issue. Thanks for checking it out and replying.

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