-
Notifications
You must be signed in to change notification settings - Fork 922
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
Conversation
- give more room for content - make less clunky overlaps when not enough room
(fixes a clear:both issue on small screens)
(involves slightly rejigging the way printable_name() invokes i18n)
👍 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 { |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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
.
There was a problem hiding this comment.
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.
Thanks all for the comments. I've just pushed some changes which I think fix all issues, except for the |
ul.secondary-actions.pager { | ||
float: right; | ||
} | ||
} |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
After fixing the CSS there are three other test failures, likely caused by changes in the HTML that means the tests need updating:
|
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. |
A couple of visual issues I spotted. Firstly a place where a Secondly, on the changeset page the pagination controls are hitting the bottom of the coloured bar at the top: 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. |
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: The other issue, will need to check. Possibly just a clearfix or something, I remember having a similar alignment issue recently. |
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. |
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: I've put this tweak in a separate branch - votes for/against putting it into this PR welcome. |
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. |
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? |
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. |
Thanks @danstowell for your work here -- it definitely influenced our work on the browse pages for the redesign. |
This pull request is for a mild rejig of the
/browse/
pages to make them a bit more approachable.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...