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

Simplify markup and styling of admin header #1611

Merged
merged 10 commits into from Jul 22, 2014
Merged

Simplify markup and styling of admin header #1611

merged 10 commits into from Jul 22, 2014

Conversation

@fofr
Copy link
Contributor

@fofr fofr commented Jul 22, 2014

  • The primary navigation of Whitehall is nothing like a Bootstrap navbar. If we don’t have to override all those Bootstrap styles the CSS can be simplified a lot.
  • This also fixes the rendering in IE7 when all tabs are visible
  • No longer hides tabs at smaller page widths
  • Keeps the header looking as it did before, minus the horrible text shadows and pipes (|) that would underline on hover
  • Remove or clarify CSS that was also in layout.scss

Implementation:

  • Use a new masthead class, and split into two sections: details and tabs.
  • Simplify markup by removing nested containers
  • Make visuallyhidden a re-usable class by taking it out of the nav specific styles

Before:
screen shot 2014-07-18 at 18 21 23

After:
screen shot 2014-07-18 at 18 21 09

Collapsed:
screen shot 2014-07-21 at 15 30 57
screen shot 2014-07-21 at 15 31 08

fofr added 3 commits Jul 18, 2014
* Styles made all span2 classes expand to 100% at small widths
* Instead, do this explicitly with a new class

(Bootstrap 3 has proper hooks for doing this)
* There are no nav-headers being used in the admin app anywhere
* The primary navigation of Whitehall is nothing like a Bootstrap
navbar. If we don’t have to override all those Bootstrap styles the CSS
can be simplified a lot.
* Use the `masthead` class, and split into two sections, details and
tabs.
* Simplify markup by removing nested containers
* Make visuallyhidden a re-usable class by taking it out of the nav
specific styles
* Keep the header looking exactly as it did before (excluding the
horrible text shadows)
@fofr

This comment has been minimized.

Copy link
Contributor Author

@fofr fofr commented on 422d260 Jul 18, 2014

Before:
screen shot 2014-07-18 at 18 21 23

After:
screen shot 2014-07-18 at 18 21 09

@edds

This comment has been minimized.

Copy link
Contributor

@edds edds commented on app/views/layouts/admin.html.erb in 422d260 Jul 21, 2014

Can this anchor link point at the element it is toggling. I can't actually see where #navigation would be in this template.

This comment has been minimized.

Copy link
Contributor Author

@fofr fofr replied Jul 21, 2014

I can improve that too, yes. Or, perhaps better, link to a New Document page that isn't JavaScript dependent?
I've just noticed the "More" and "New document" dropdowns are linking to the same #navigation element.

This comment has been minimized.

Copy link
Contributor

@edds edds replied Jul 21, 2014

Eye. It was them both linking to the same element that made me notice. Less JS dependancy would be A+++

fofr added 4 commits Jul 19, 2014
* Use tab and menu item specific classes for clearer selector intent
* Make ‘All users’ and ‘Import’ links a standard link_to, on preview
there’s a display bug when these links are active
* Be clearer about when a link is for a menu or a tab
* Each drop down menu link should point to the ID of the element it
displays (see
422d260
820474e62d77d)
* Ideally there should be some non-js support but that can come later.
* Add ARIA roles to menus as recommended by Bootstrap 3 drop down patterns.
* ‘more-nav’ is a class that should really only apply to the “More” tab
* Use a more descriptive and re-usable classname, ‘masthead-menu’, also
conforms to the ‘masthead-menu-item’ class and its aria role.
* Previously we’d hide the “Corporate information” tab at tablet width
and below. This is bad form.
* The CSS has already been removed, but the class should be removed too.
@include ie-lte(7) {
display: inline;
zoom: 1;
}

This comment has been minimized.

@edds

edds Jul 22, 2014
Contributor

Incase you didn't know we have a mixin to help with inline-block for IE: https://github.com/alphagov/govuk_frontend_toolkit/blob/master/stylesheets/_shims.scss#L6-L35

This comment has been minimized.

@fofr

fofr Jul 22, 2014
Author Contributor

Thanks for the heads up. Updated and rebased.

@edds

This comment has been minimized.

Copy link
Contributor

@edds edds commented on d969847 Jul 22, 2014

Thank you.

fofr added 3 commits Jul 21, 2014
* IE7 is ~9% of current users, currently (on the live version) the tabs
aren’t displaying at all
* Use inline-block mixin for inline list to ensure it works in older
versions of IE
* Collapse tabs into a stacked view using media queries
* Breakpoint determined by the point at which wrapped text within tabs
causes problems (slightly above the usual tablet breakpoint)
* Shift margins in IE7 for correct alignment
* Remove old media query, the indicator margin doesn’t need to change
at smaller widths
edds added a commit that referenced this pull request Jul 22, 2014
Simplify markup and styling of admin header
@edds edds merged commit 9f0d6c5 into master Jul 22, 2014
1 check passed
1 check passed
default Build #3536 succeeded on Jenkins
Details
@edds edds deleted the tab-clarity branch Jul 22, 2014
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

2 participants
You can’t perform that action at this time.