Diary cleanup #218

Closed
wants to merge 12 commits into
from

Conversation

Projects
None yet
6 participants
Member

samanpwbb commented Mar 14, 2013

This commit includes a few basic changes to make the User Diaries friendlier and easier to read. I adjusted the text styles and added a maximum width, to keep the line length under control

Here's a screenshot:

Screen Shot 2013-03-14 at 6 18 30 PM

While working on the diary, I also modernized the markdown reference sheet with better markup and clearer styles:

Screen Shot 2013-03-14 at 6 19 49 PM

Contributor

lxbarth commented Mar 21, 2013

Looking forward to see this land - this will make posts like this one much neater :)

http://www.openstreetmap.org/user/lxbarth/diary/18851

@tomhughes - how can we help move this forward?

Contributor

ingalls commented Mar 21, 2013

I second this, can't wait to see it in action!

Owner

tomhughes commented Mar 22, 2013

Sorry, been quite busy the last couple of weeks so I haven't had a chance to look at the code yet - will try and do so over the weekend.

I did push this to http://tomh.dev.apis.openstreetmap.org/ a few days ago though if people want to have a look at it running live and see what they think. It looks pretty good to me though.

Contributor

lxbarth commented Mar 22, 2013

@tomhughes - thanks man. Is there a typo in URL? Doesn't resolve for me.

Try http://tomh.apis.dev.openstreetmap.org/

On 22 Mar 2013, at 12:06, Alex Barth notifications@github.com wrote:

@tomhughes - thanks man. Is there a typo in URL? Doesn't resolve for me.


Reply to this email directly or view it on GitHub.


rails-dev mailing list
rails-dev@openstreetmap.org
http://lists.openstreetmap.org/listinfo/rails-dev

Contributor

lxbarth commented Mar 22, 2013

Works. My lxbarth user just got suspended when I tried testing (logged in, created a diary entry) - can you unlock? Thanks!

http://cl.ly/image/223s133p0j3F

Owner

tomhughes commented Mar 22, 2013

Unlocked.

Contributor

lxbarth commented Mar 22, 2013

@tomhughes tomhughes and 1 other commented on an outdated diff Mar 24, 2013

app/views/diary_entry/_diary_entry.html.erb
@@ -4,7 +4,16 @@
<%= user_thumbnail diary_entry.user %>
<% end %>
- <h2><%= link_to h(diary_entry.title), :action => 'view', :display_name => diary_entry.user.display_name, :id => diary_entry.id %></h2>
+ <h2>
+ <% if params[:action] == 'list' %>
+ <%= link_to h(diary_entry.title), :action => 'view', :display_name => diary_entry.user.display_name, :id => diary_entry.id %>
+ <% else %>
+ <%= diary_entry.title %>
+ <% end %>
+ </h2>
+ <small class='deemphasize'>
@tomhughes

tomhughes Mar 24, 2013

Owner

What's the win here? Do we just think it's "prettier" if the title isn't a link on the diary entry view? It's adding a very kludgy complication to the template, and removing a handy link that can be used to grab a link to the diary entry so I'd like there to be a decent upside to make it a worthwhile change.

@samanpwbb

samanpwbb Mar 24, 2013

Member

Yep, I will revert this. No real win and it is unnecessary complexity.

Visually, that strong "link" blue on post on titles doesn't work. It feels a little weird. There are other alternatives, like adding a little permalink icon or having more nuanced link styles across the board, would be more appropriate.

@tomhughes tomhughes commented on the diff Mar 24, 2013

app/views/site/_markdown_help.html.erb
@@ -1,29 +1,26 @@
-<table>
- <thead>
@tomhughes

tomhughes Mar 24, 2013

Owner

I know you hate tables and love lists, but please, just how is this better? I know the result looks nicer, but could we not achieve the same result with the table - it is basically naturally tabular information after all, and the CSS needed to lay the list out as a table is just horrendous and fragile in the extreme.

@samanpwbb

samanpwbb Mar 24, 2013

Member

This is definitely a place where a table isn't wrong semantically. The main reason I have been replacing tables with more "generic" CSS elements is to enable increased layout control. With this kind of markup, I can move the headings on top of the paragraphs, or move headings to the left with floats, or implement a completely new layout without changing the markup at all. If OSM.org hopes to evolve into a more modern website, moving away from tables as much as possible is a must.

@tomhughes

tomhughes Mar 26, 2013

Owner

So the issue is that CSS doesn't give you as much control of table styling as it does of styling a structure like this? I guess that is a reasonable reason... It was just all the fixed pixel sizes that the new scheme relies on seemed quite fragile to me, but I can't really claim to be an expert in such things.

Owner

tomhughes commented Apr 2, 2013

Any news @samanpwbb? I think there is @lxbarth's comment outstanding and you said you were going to revert the change re the heading being a link?

Member

samanpwbb commented Apr 2, 2013

@tomhughes I reverted the heading change, fixed the images, and also cleaned up the markdown help box code.

Owner

tomhughes commented Apr 3, 2013

I've pushed those changes into the test site at http://tomh.apis.dev.openstreetmap.org/ and it all looks pretty good to me now.

I think the only thing I personally would object to is the 740px width restriction on diary entries, but I suspect I may be in a bit of a minority on such things so I'm willing to be guided by what other people think.

Member

samanpwbb commented Apr 10, 2013

Without the limit, it was easy to hit really unwieldy line lengths. Some background on readability and line length.

Anything else you want me to look at here?

Owner

tomhughes commented Apr 14, 2013

Oh I understand the idea of limiting the line length, I just hate the resulting white space at the right hand side ;-)

Nobody else seems to have any comments anyway, so I'm going to go ahead and merge thus.

Owner

tomhughes commented Apr 14, 2013

Merged.

tomhughes closed this Apr 14, 2013

Contributor

harry-wood commented Apr 18, 2013

There's no way to make images appear side-by-side now.

And it's screwed up the layout of my images (photo thumbnails) wherever I was arranging them side-by-side in the past. e.g.: http://www.openstreetmap.org/user/Harry%20Wood/diary/13733

...but maybe I'm the only one doing that :-)

Contributor

lxbarth commented Apr 24, 2013

Awesome to see this merged. Thanks so much.

@harry-wood - I opened a new ticket for the issue you reported, not sure whether this is a regression: #231

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