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

New single post view #4410

Merged
merged 39 commits into from Aug 22, 2013

Conversation

Projects
None yet
@rogerbraun
Copy link
Contributor

rogerbraun commented Aug 17, 2013

This is about this bug: #3741

I implemented most of the proposal here. As you can see, it is based on #4390, as I require the header changes anyway.

The implementation is pretty straightforward and tries to reuse as much from other components/views as possible, so this has a working lightbox, oembed etc. Please take a look at it and comment on questionable/wrong/erroneous parts.

My placement of the css code is probably not quite right, I don't really understand the SCSS structure and would appreciate it if someone could take a look at it.

Specs still run after small changes.

Things that are missing or have to reworked:

  • Vestiges of the old SPV have to be removed.
  • CSS for oEmbed videos needs to be adjusted.
  • Micro pictures of commented users are not unique, so a user that does three comments will appear three times. I don't know if this should be changed or not.
  • Probably more specs should be written, but I'm not that familiar with jasmine. Maybe someone else can take a look at it or point me in the right directions.
  • Comments/Posts can not be removed.
  • The post always displays, nsfw or not.
  • The vertical line between post content and interactions is only as high as the post content div. This needs some css hacking.
  • Hovercards don't work quite right.

Having said that, it is already much nicer to use than the old SPV. Please review, so we can get it merged soon.

Here are some images that show the current look.
spv3
spv2
spv1

@rogerbraun

This comment has been minimized.

Copy link
Contributor Author

rogerbraun commented Aug 17, 2013

Just remembered, hovercards are broken, too.

@jhass

This comment has been minimized.

Copy link
Member

jhass commented Aug 17, 2013

Nice! Some first rough design points:

  • Remove the boxes around the icon buttons, they do not fit into the design anymore.
  • The icons on the right side are meant informational and not for interaction, the interaction should happen through the series of icons on the post side, where you only have the like icon at the moment. The comment icon there should focus the comment textarea, much like the 'Comment' link in the stream currently.
  • Location data isn't display
  • I think it would be nice if the comment textarea spans over the full right side.
  • SInce the right side icons are only informational, they shouldn't be display if there's nothing to show (no likes row if there are 0 likes, no reshares row if there are 0 reshares).
  • The avatar of the post author should be the normal one, not the completely rounded version of the current SPV.
@jhass

View changes

app/assets/stylesheets/single-post-view.css.sass Outdated
.comment
border-bottom: solid 1px #ccc
padding-top: 10px
padding-bottom: 10px

This comment has been minimized.

@jhass

jhass Aug 17, 2013

Member

It would be nice if you could convert that to SCSS as we're trying to drop SASS from the codebase currently. Don't worry too much of the location, there's a restructuring planned for all this anyway.

@rogerbraun

This comment has been minimized.

Copy link
Contributor Author

rogerbraun commented Aug 17, 2013

The icons on the right ARE only informational already ;-). There is actually both a like and a reshare button on the left, but the reshare button does not display because the post can't be reshared.

@jhass

This comment has been minimized.

Copy link
Member

jhass commented Aug 17, 2013

Ah, my bad. So just a comment button focusing the comment textarea is missing :)

@Flaburgan

This comment has been minimized.

Copy link
Member

Flaburgan commented Aug 17, 2013

The icons on the right side are meant informational and not for interaction, the interaction should happen through the series of icons on the post side, where you only have the like icon at the moment.

In my mockups I removed the post side icons and only use the right side to inform and interact.

@jhass

This comment has been minimized.

Copy link
Member

jhass commented Aug 17, 2013

@jhass

This comment has been minimized.

Copy link
Member

jhass commented Aug 17, 2013

@rogerbraun oh and about collapsing multiple comments into one, that's a nice idea but not scope of this PR since it would need to consistently happen in the streams too.

@rogerbraun

This comment has been minimized.

Copy link
Contributor Author

rogerbraun commented Aug 17, 2013

@MrZyx I don't mean collapsing the actual comments, I mean only the small user pictures that are displayed next to the comment count. If a user comments x times, her image will show up x times. This can be considered a bug or a feature, I don't really mind either way.

@jhass

This comment has been minimized.

Copy link
Member

jhass commented Aug 17, 2013

Yeah, I meant that too basically :) I'd consider removing the avatars as a feature since it requires extra logic and isn't current behavior anywhere else.

A possible CSS trick for the middle border could be enabling it on both sides (border-right / border-left respectively), replacing margin-left/-right with padding-left/-right respectively, setting the margins to 0 and then offsetting one side with position: relative and left: -1px; or right: 1px; respectively.

@jhass

This comment has been minimized.

Copy link
Member

jhass commented Aug 17, 2013

Btw. if you want a build on Travis you need to rebase against latest develop.

@goobertron

This comment has been minimized.

Copy link

goobertron commented Aug 17, 2013

Looking good! Thanks for taking this on.

we decided on https://wiki.diasporafoundation.org/File:Spv-sean-jonne.png didn't we?

We did indeed, in this proposal, in which the 'abstain' option (the majority) was for the mock-up above.

My feeling re the avatars for comments is that, as the comments are listed below, there is no need for a separate row of avatars of people who have commented. I'd remove that row, and just have a row for reshares (if any), a row for likes (if any), and then a stream of comments, with user name and avatar by each comment as you have already.

@jhass

This comment has been minimized.

Copy link
Member

jhass commented Aug 17, 2013

@goobertron right, the mockup does it that way already.

@rogerbraun

This comment has been minimized.

Copy link
Contributor Author

rogerbraun commented Aug 17, 2013

Hah, you're right. I did not realize that the avatars were missing for the comments... I'll remove them. BTW, if someone else wants to move stuff around / fix the css, I would not mind at all ;-)

@svbergerem

This comment has been minimized.

Copy link
Member

svbergerem commented Aug 17, 2013

Nice work! I just wanted to add another point to the to-do list ;) The authors avatar should look like any others avatar (without the shadow and the round border)

@rogerbraun

This comment has been minimized.

Copy link
Contributor Author

rogerbraun commented Aug 17, 2013

Travis is failing again for random specs... Don't know the reason, the specs run fine locally.

@jhass

This comment has been minimized.

Copy link
Member

jhass commented Aug 17, 2013

We do have couple of random failures in the cucumber suite, the jasmine timeout is still unique to the commit from the header PR.

@Flaburgan

This comment has been minimized.

Copy link
Member

Flaburgan commented Aug 17, 2013

The authors avatar should look like any others avatar (without the shadow and the round border)

If the element has the avatar class, it'll be enough when the PR will be merged

@rogerbraun

This comment has been minimized.

Copy link
Contributor Author

rogerbraun commented Aug 17, 2013

Adding location is a bit hard, as it can get very long. for example, I have "Posted from: Adulis, Lederstraße, Rathaus, Stuttgart-Mitte, Stuttgart, Regierungsbezirk Stuttgart, Baden-Württemberg, 70173, Germany, European Union". This doesn't fit.

@jhass

This comment has been minimized.

Copy link
Member

jhass commented Aug 17, 2013

Why not just a max-width div with about 30% and let it wrap?

@rogerbraun

This comment has been minimized.

Copy link
Contributor Author

rogerbraun commented Aug 17, 2013

It's actually pretty useless with such a long location string. It really has to be reduced to something like 'Stuttgart, Germany" to be useful at all. Here's the current look.

spv4

@rogerbraun

This comment has been minimized.

Copy link
Contributor Author

rogerbraun commented Aug 17, 2013

It's especially useless as the location isn't even right, only near.

@jhass

This comment has been minimized.

Copy link
Member

jhass commented Aug 17, 2013

well, I'm sure that wouldn't look that bad if you change to the normal avatar, make the avatar slightly bigger, reduce the font size, make it gray, increase the hearts icon size and set off the post content like in the mockup.

We have that long strings in the system now, we need to find a way to deal with them.

@Flaburgan

This comment has been minimized.

Copy link
Member

Flaburgan commented Aug 17, 2013

We have that long strings in the system now, we need to find a way to deal with them.

I also think the location should be less precise, but this should not be done in this PR

@rogerbraun

This comment has been minimized.

Copy link
Contributor Author

rogerbraun commented Aug 17, 2013

spv5

I'm not convinced... But better location handling is out of the scope of this anyway.

Regarding the icons: As far as I can see there is now way to easily scale them up or color them, as they are images. Are there plans to switch to font-awesome or something?

@DeadSuperHero

This comment has been minimized.

Copy link
Member

DeadSuperHero commented Aug 17, 2013

This is starting to look really good! A few things:

  1. Would it be possible to make images fill up the width of the left side?
  2. The location field could probably be improved by making its width fill the left side, too.
  3. How is scrolling anticipated to work? Do both sides scroll together, or can both sides scroll independently?
  4. Similarly, the comment pane on the right side could probably be a bit longer to complement the width of the div it's in.

Other than those small nitpicks, I'm really thrilled to see how this is coming along!

@jhass

This comment has been minimized.

Copy link
Member

jhass commented Aug 17, 2013

Nothing too concrete, you can find a SVG versions of the heart in the graphics/ folder, I fear for comment and reshare someone has to recreate them. @DeadSuperHero where do you have the large ones in the mockup from?

@DeadSuperHero

This comment has been minimized.

Copy link
Member

DeadSuperHero commented Aug 17, 2013

@MrZyx It's been a while since I last worked on them, but I'm pretty sure I just used Entypo for the mockup.

@svbergerem

This comment has been minimized.

Copy link
Member

svbergerem commented Aug 17, 2013

IMO just the location should be grey, the rest should be black. The avatar should be a square like the other avatars perhaps use thumb_medium_... for the authors avatar and thumb_small_... for the others. We could also think about putting the location under the post instead and then let it fill the left side

@rogerbraun

This comment has been minimized.

Copy link
Contributor Author

rogerbraun commented Aug 17, 2013

spv6

I made the left part wider, it really does not look that stupid anymore ;-)

Right now, both fields scroll together. I think this should be enough for now, but separate scrolling might be a nice addition.

The other suggestions are all good, I'll try to implement them.

@rogerbraun

This comment has been minimized.

Copy link
Contributor Author

rogerbraun commented Aug 22, 2013

Merge away!

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

Merge pull request #4410 from rogerbraun/feature/spv-redesign
New single post view

Conflicts:
	app/assets/stylesheets/application.css.sass

@jhass jhass merged commit 96436c5 into diaspora:develop Aug 22, 2013

@jhass

This comment has been minimized.

Copy link
Member

jhass commented Aug 22, 2013

Alright, thanks everybody!

@rogerbraun

This comment has been minimized.

Copy link
Contributor Author

rogerbraun commented Aug 22, 2013

Really happy to see this get merged!

Chat is next ;-)

@DeadSuperHero

This comment has been minimized.

Copy link
Member

DeadSuperHero commented Aug 22, 2013

Woohoo!

On Thursday, August 22, 2013, Jonne Haß wrote:

Alright, thanks everybody!


Reply to this email directly or view it on GitHubhttps://github.com//pull/4410#issuecomment-23075756
.

@svbergerem

This comment has been minimized.

Copy link
Member

svbergerem commented Aug 22, 2013

Hooray! \o/

@jhass jhass referenced this pull request Aug 22, 2013

Closed

Polish single post view #4425

2 of 3 tasks complete
@Flaburgan

This comment has been minimized.

Copy link
Member

Flaburgan commented Aug 22, 2013

yeeeepeeee!!! Thank you everyone, especially @rogerbraun who comes and kick us to move on that point, and to @svbergerem who did a nice job too.

@movilla

This comment has been minimized.

Copy link
Contributor

movilla commented Aug 22, 2013

Amazing work!

@goobertron

This comment has been minimized.

Copy link

goobertron commented Aug 22, 2013

Wonderful! Thank you all of you.

I wonder whether this is the longest issue discussion in Diaspora's github? It's been fascinating and great to see how the collaboration has been working.

@fabianrbz

This comment has been minimized.

Copy link
Contributor

fabianrbz commented Aug 22, 2013

nice work!

@Flaburgan Flaburgan referenced this pull request Jun 4, 2015

Closed

Port to Bootstrap 3 #6015

13 of 13 tasks complete
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.