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

Mild rejig of the browse pages #487

Closed
wants to merge 28 commits into from

Conversation

danstowell
Copy link
Contributor

This pull request is for a mild rejig of the /browse/ pages to make them a bit more approachable.

  • Last-edited information is now in the form "Edited [x] ago by [x] - (version [x] in changeset [x])" rather than a tabulation of the attributes
  • Tweaked the ordering in the heading eg "Relation: Liverpool Street Station (1571755)" becomes "Liverpool Street Station (Relation 1571755)"
  • Tried to make the tags section a bit more primary, i.e. the first thing a new visitor might alight upon, rather than confusing things such as changesets or XML
  • Didn't remove anything!

There's a bit more blurb, and some screenshots, on my blog page about it.

I've made a couple of changes to language-strings, which would imply translation needed eventually.

I'm aware there might be deeper changes afoot (sidebar changes etc). I presume my changes are superficial enough that they shouldn't have any impact on larger plans others may have...

pnorman added a commit to osmlab/openstreetmap-upcoming-features that referenced this pull request Sep 24, 2013
@jfirebaugh
Copy link
Member

👍 I like these changes, and I think they will fit well with future sidebar changes. I have a few code review type comments which I'll do line by line, but overall I think this looks good.

@@ -1 +1,5 @@
/* Styles specific to large screens */
ul.secondary-actions.pager {
Copy link
Member

Choose a reason for hiding this comment

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

I think we'd like to just delete large.css, so can this go in the main stylesheet?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I assume the motivation for deleting large.css is for speed of loading?
On small screens, the float I added looks confusing and messy, which is why I moved it to large-only. If I simply put it in common.css and attempt to override it with float:none in small.css, it doesn't override, for me. (I tend to avoid using !important because it feels hacky.) Suggestions welcome about what would be best here.

Copy link
Member

Choose a reason for hiding this comment

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

I think the plan was to get rid of the whole small/large thing in due course and just use media queries in the main file when the site can't be made responsive in other ways, but I might be wrong.

Historically we haven't used large.css that much - in most cases we just put the large screen code in common.css and override it for small screens in small.css.

Copy link
Member

Choose a reason for hiding this comment

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

Can we just drop this change? It seems pretty minor, and on /browse/changesets, it results in the top pager being hidden behind the map.

@danstowell
Copy link
Contributor Author

Thanks all for the comments. I've just pushed some changes which I think fix all issues, except for the large.css question - see above - if there's a better way to do it within this PR (i.e. without doing anything major to the way small/large is currently done) then I'd be happy to...

ul.secondary-actions.pager {
float: right;
}
}
Copy link
Member

Choose a reason for hiding this comment

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

Regardless of any other issues with this style rule, you have more close braces then open, which is causing a large number of test failures.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Oh dear, that's embarrassing, thanks. Will push a brackets fix immediately, before considering the other remaining issues.

@tomhughes
Copy link
Member

After fixing the CSS there are three other test failures, likely caused by changes in the HTML that means the tests need updating:

  1) Failure:
BrowseControllerTest#test_redacted_node_history [/home/tom/src/osm/rails/test/functional/browse_controller_test.rb:100]:
Expected exactly 2 elements matching "body div#content div.browse_details", found 4..
Expected: 4
  Actual: 2

  2) Failure:
BrowseControllerTest#test_redacted_relation_history [/home/tom/src/osm/rails/test/functional/browse_controller_test.rb:126]:
Expected exactly 4 elements matching "body div#content div.browse_details", found 8..
Expected: 8
  Actual: 4

  3) Failure:
BrowseControllerTest#test_redacted_way_history [/home/tom/src/osm/rails/test/functional/browse_controller_test.rb:112]:
Expected exactly 4 elements matching "body div#content div.browse_details", found 8..
Expected: 8
  Actual: 4

714 tests, 247420 assertions, 3 failures, 0 errors, 0 skips

@danstowell
Copy link
Contributor Author

Thanks all. Updated the test suite, updated the locales a little, and improved the added CSS (making the large.css unneeded). I hope it looks OK to you.

@tomhughes
Copy link
Member

A couple of visual issues I spotted. Firstly a place where a Comment: label does not line up with the text:

screenshot from 2013-10-13 19 43 55

Secondly, on the changeset page the pagination controls are hitting the bottom of the coloured bar at the top:

screenshot from 2013-10-13 19 42 21

I suspect this is related to the fact that you haven't moved the changeset summary information into the header like you have on the node/way/relation pages.

@danstowell
Copy link
Contributor Author

Thanks @tomhughes. I was unsure about making the same move for the changeset information, but prompted by your comment I realise it needs to be updated to match (for UI consistency's sake). Here's how I've done it:

changesetdates

The other issue, will need to check. Possibly just a clearfix or something, I remember having a similar alignment issue recently.

@goldfndr
Copy link
Contributor

goldfndr commented Nov 2, 2013

Has consideration been given to changing "Version [v] in changeset [c]" to "Changeset [c] has version [v]" (or something similar)? The version number binds tightly to the View History link, the changeset number does not.

@danstowell
Copy link
Contributor Author

Thanks @goldfndr - I do see what you mean, but the phrase you suggest isn't very fluid in English. I haven't thought of a neat English phrasing that would put the version next to the history link, and various ways to put the history link inside tend towards awkward lego.

Here's one way:

screenshot from 2013-11-03 10 27 39

I've put this tweak in a separate branch - votes for/against putting it into this PR welcome.

@danstowell
Copy link
Contributor Author

OK, merged that slight tweak into this PR, I like it better. I'm cosy with this PR right now, but do let me know any issues spotted.

@jfirebaugh jfirebaugh mentioned this pull request Nov 12, 2013
18 tasks
@tomhughes
Copy link
Member

The redesign has now been merged (though not deployed just yet), which I suspect makes this mostly redundant, or at least in need of a rework. I know @jfire said he had made some changes to the redesign based on your work here.

Sorry we didn't actually manage to get this merged at all, but are you happy to close this now?

@danstowell
Copy link
Contributor Author

I'm happy that some of the ideas influenced #498. And clearly there's no future for merging this branch. If I have any feelings about making the new browse pages more approachable, I'll have a go. Thanks all for the input.

@danstowell danstowell closed this Nov 28, 2013
@jfirebaugh
Copy link
Member

Thanks @danstowell for your work here -- it definitely influenced our work on the browse pages for the redesign.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants