Skip to content

Conversation

@Talina06
Copy link
Contributor

@oalders : I have made these updates to the existing plussers display so far. Please review the branch and let me know about the subsequent changes.

Copy link
Member

Choose a reason for hiding this comment

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

We should avoid inline styles wherever possible. They are already in lots of templates, but that's something we should move away from. So, the font size should go into the CSS.

@oalders
Copy link
Member

oalders commented Jun 14, 2014

@Talina06 This looks really good, but I think we need to address the issues I mentioned before we can merge this. I pushed a couple of commits as well.

@Talina06
Copy link
Contributor Author

@oalders : Could you have a look at the changes I have pushed?
In case of further editing please let me know.

@oalders
Copy link
Member

oalders commented Jun 24, 2014

This answer suggests that having the height and width inline are preferable, unless the image is actually in the background. http://stackoverflow.com/questions/640190/image-width-height-as-an-attribute-or-in-css

@Talina06
Copy link
Contributor Author

@oalders Oh I was unaware of that bit of detail.
Thanks! I will move it back.

@oalders
Copy link
Member

oalders commented Jun 25, 2014

  1. Could we centre the PAUSE id under the gravatar? With shorter names, it looks a bit off.

screenshot 2014-06-24 22 06 10

  1. Also, I've noticed there's no page title. So, if you're not looking at the URL, it's hard to know exactly what the page is for.

  2. There's a fair amount of horizontal scrolling. I'm at 1280 x 800 and in Chrome I have to scroll a lot.

Have you looked at this on mobile at all? I'm curious as to how it looks there.

Sorry for not noticing this before. :(

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@oalders : I had to add the height and width to the css too because this particular image wasn't getting sized properly.

rsz_screenshot_from_2014-06-28_175750

@Talina06
Copy link
Contributor Author

@oalders: I have improved the horizontal scrolling and the alignment of PAUSE id under the images.

Also, I tried testing for mobile by reducing the browser size and it looks something like this.
Needs any editing?

rsz_screenshot_from_2014-06-28_183812 1

oalders added a commit that referenced this pull request Jun 30, 2014
Displaying plussers on a separate page.
@oalders oalders merged commit 3ff6c4c into master Jun 30, 2014
@oalders
Copy link
Member

oalders commented Jun 30, 2014

@Talina06 Thanks! I added a couple of commits before merging. Please review them. :)

@oalders oalders deleted the talina/plusser branch July 3, 2014 04:50
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

3 participants