Merge lp://staging/~ronnie.vd.c/ubuntu-website/top-nav into lp://staging/~ubuntu-website-community/ubuntu-website/light-base-theme

Proposed by Ronnie
Status: Merged
Merge reported by: Chris Johnston
Merged at revision: not available
Proposed branch: lp://staging/~ronnie.vd.c/ubuntu-website/top-nav
Merge into: lp://staging/~ubuntu-website-community/ubuntu-website/light-base-theme
Diff against target: 75 lines (+37/-3)
2 files modified
css/default.css (+15/-3)
index.html (+22/-0)
To merge this branch: bzr merge lp://staging/~ronnie.vd.c/ubuntu-website/top-nav
Reviewer Review Type Date Requested Status
KuZeKo - Matteo Lissandrini (community) Approve
Ronnie (community) Needs Resubmitting
Michael Hall (community) Approve
Review via email: mp+51312@code.staging.launchpad.net

Description of the change

Added top-nav

To post a comment you must log in.
Revision history for this message
KuZeKo - Matteo Lissandrini (kuzeko) wrote :

The top-nav should use a better, *more* semantic code.

review: Disapprove
Revision history for this message
Ronnie (ronnie.vd.c) wrote :

KuZeKo, what do you mean by "*more* semantic code"

Revision history for this message
KuZeKo - Matteo Lissandrini (kuzeko) wrote :

Without all those "span"s , using a "nav" or a "ul>li>a" etc.

8. By Ronnie

removed span elements in top-nav

Revision history for this message
Ronnie (ronnie.vd.c) wrote :

Changed the span elements to more descriptive HTML tags

review: Needs Resubmitting
9. By Ronnie

Added nav and section tags instead of span

Revision history for this message
KuZeKo - Matteo Lissandrini (kuzeko) wrote :

Far better. :-)

consider using <span class="{something}">|</span> and converting the top-nav in aside

10. By Ronnie

changed to <aside> and put | between <span>

11. By Ronnie

Added some extra spacing, and changed font-type comment

Revision history for this message
Stas Sușcov (sushkov) wrote :

Is this a fix for a bug or something? Afaik the loco theme/websites do not necessary require the top navigation bar. It was introduced on planet, and imho there it should stay: on third party loco services, not on loco webpages.

Reasons:
1. It will confuse people. http://ubuntu.ro/ has a planet, a news page and a launchpad project, by placing the introduced by you navigation bar, how do I help user to make the correct decision which link he should follow?
2. It is useless on most of the loco websites, since not everyone has a wiki/planet/news page, and they usually have a "placeholder" that redirects visitors anyway to ubuntu.com
3. I don't see why I need a login/logout link above my main menu.

I wouldn't merge this into WordPress theme, since I don't see how it can help end-users, and as a result, I will not support this merge into base theme. But, I encourage you to write a patch to the themes for planet/forum, where it makes sense to have a top bar like that.

Revision history for this message
Ronnie (ronnie.vd.c) wrote :

There is al least one bug in website-content for it https://bugs.launchpad.net/ubuntu-website-content/+bug/680814. The request came from loco.ubuntu.com, but why not share the code with the whole community. Its like all the other code in the base-theme: use it when you need it, leave it out when you dont need.

I can imagine there are loco's who would also like to have a top bar. So why not include it in the base-theme. Also, each loco/website can decide which links they place in the top-nav, as long the top-nav is consistent on the whole domain.

Also the login is optional, it is possible to place the login on other parts of the webpage, the base-theme does not force you to have it in the top-left corner. But if the domain has a lot of different apps, then i think its good to have to login button on the same place each page (single sign-on should be awsome, but thats usually too much work for a loco)
NOTE: a user can be confused when he/she is logged-in at one app, but needs to login on the other.

Revision history for this message
KuZeKo - Matteo Lissandrini (kuzeko) wrote :

[ Quote Stas Sușcov ]
> Is this a fix for a bug or something? Afaik the loco theme/websites do not
> necessary require the top navigation bar. It was introduced on planet, and
> imho there it should stay: on third party loco services, not on loco webpages.

As they say "Melius abundare quam deficere", if we can have this as an optional element, go for it. There are already a couple of "optional" elements in this theme.

Revision history for this message
Michael Hall (mhall119) wrote :

It was my understanding that light-base-theme was there to provide a common way of implementing any particular design feature. As long as there is more than one site that might want this, it should be in there so that we're not duplicating effort or, worse, making it different on every site.

I will be merging this into light-django-theme, and hope that it is merged into the others as well.

review: Approve
12. By Ronnie

top-nav links are now with http://

Revision history for this message
Ronnie (ronnie.vd.c) wrote :

Links were realtive, ill made them absolute by adding http://

review: Needs Resubmitting
Revision history for this message
KuZeKo - Matteo Lissandrini (kuzeko) :
review: Approve
13. By Ronnie

top-nav is now white instead of grey

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
The diff is not available at this time. You can reload the page or download it.

Subscribers

People subscribed via source and target branches