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.

Sign up for GitHub

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

Improve admin header #959

Closed
wants to merge 2 commits into from
Closed

Improve admin header #959

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
77ecbe7
Convert admin content-header into a fixed header
Murph33 Mar 3, 2016
298fda2
Move header texts out of 16 column skeleton
Murph33 Mar 3, 2016
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
13 changes: 12 additions & 1 deletion backend/app/assets/stylesheets/spree/backend/shared/_layout.scss
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -65,6 +65,10 @@ body {
@include clearfix;
}

.content-under-header {
padding-top: 89px;
Copy link
Contributor

Choose a reason for hiding this comment

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

What does this value represent? If it's the header's height we should save that as a value: $height-header and use it here and on the header.

}

// Header
//---------------------------------------------------
.admin-nav-header {
Expand Down Expand Up @@ -103,9 +107,16 @@ body {
}

#content-header {
padding: 15px 0;
padding: 15px 14px;
background-color: very-light($color-3, 4);
border-bottom: 1px solid $color-border;
position: fixed;
left: $width-sidebar;
width: calc(100% - #{$width-sidebar});
z-index: 1;
@include display(flex);
@include align-items(center);
@include justify-content(space-between);
Copy link
Contributor

Choose a reason for hiding this comment

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

Let's just reorganize this a little bit:

@include display(flex);
@include align-items(center);
@include justify-content(space-between);
position: fixed;
left: $width-sidebar;
width: calc(100% - #{$width-sidebar});
padding: 15px 14px;
border-bottom: 1px solid $color-border;
background-color: very-light($color-3, 4);
z-index: 1;


.page-title {
font-size: 20px;
Expand Down
24 changes: 7 additions & 17 deletions backend/app/views/spree/admin/shared/_content_header.html.erb
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,23 +1,13 @@
<% if content_for?(:page_title) || content_for?(:page_actions) %>
<div id="content-header" data-hook>
<div class="container">
<div class="sixteen columns">
<div class="block-table">
<% if content_for?(:page_title) %>
<div class="table-cell">
<h1 class="page-title <%= yield :page_title_classes %>"><%= yield :page_title %></h1>
</div>
<% end %>
<h1 class="page-title <%= yield :page_title_classes %>"><%= yield :page_title %></h1>
Copy link
Contributor

Choose a reason for hiding this comment

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

Do you know what page_title_classes is used for? Not sure if we want to keep this around. It doesn't make a lot of sense to me.

Copy link
Contributor

Choose a reason for hiding this comment

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

Instead of having an empty <h1> we can tweak how we're using Flexbox.

#content-header {
  @include display(flex);
  @include align-items(center);
- @include justify-content(space-between);
}

+.page-actions {
+  @include flex-grow(1);
+  text-align: right;
+}

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This doesn't work quite like this since .page-actions is applied to the ul which is inside of that div with the toolbar class and datahook. Works when applied to the toolbar div obviously but are we keeping that? I would like to eliminate the empty h1 even though it shouldn't hurt, empty elements used to influence aren't great.


<% if content_for?(:page_actions) %>
<div class="page-actions table-cell toolbar" data-hook="toolbar">
<ul class="inline-menu">
<%= yield :page_actions %>
</ul>
</div>
<% end %>
</div>
<% if content_for?(:page_actions) %>
<div class="toolbar" data-hook="toolbar">
Copy link
Contributor

Choose a reason for hiding this comment

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

@jhawthorn is this a kind of class name that we need to keep around? I'd prefer to remove anything we're not actually using.

<ul class="page-actions inline-menu">
<%= yield :page_actions %>
</ul>
</div>
</div>
<% end %>
</div>
<% end %>
2 changes: 1 addition & 1 deletion backend/app/views/spree/layouts/admin.html.erb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -34,7 +34,7 @@

<%= render :partial => 'spree/admin/shared/content_header' %>

<div class="container">
<div class="container<%= ' content-under-header' if (content_for?(:page_title) || content_for?(:page_actions)) %>">
Copy link
Contributor

Choose a reason for hiding this comment

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

As we discussed IRL, move this to .main-content and change the conditional class name to .has-header. This is a lot clearer on why the padding-top is being added.

<div class="row">
<div class="<%= 'with-sidebar ' if content_for?(:sidebar) %>" id="content" data-hook>
<div class="<%= if content_for?(:sidebar) then 'twelve' else 'sixteen' end %> columns">
Expand Down