Skip to content
This repository has been archived by the owner. It is now read-only.

Ticket/unstable/263 new layout ready #114

Merged

Conversation

@edavis10
Copy link
Contributor

@edavis10 edavis10 commented Oct 28, 2011

edavis10 and others added 30 commits Jul 31, 2011
Conflicts:
	app/views/wiki/annotate.rhtml
	app/views/wiki/diff.rhtml
	app/views/wiki/show.rhtml
Conflicts:
	app/helpers/repositories_helper.rb
Makes bulk changes that much faster...
Taken from Redmine r5880
Committed by Jean Philippe Lang
Invalid watcher user error when adding an invalid user as watcher #577
The pdf export tried to export the initial journal, which it shouldn't.
[#573] acts_as_searchable definition in WikiPage may be insufficient and cause SQL errors
…e_gemfile

Rip faster_csv out of lib into the Gemfile. #517
(Sorry for the commit-noise)
<%# Added: classes to the li %>
<%# Added: classes to the a %>
<%# Added: div for submenu icons %>
<%# Removed: icon-* on links %>

This comment has been minimized.

@thegcat

thegcat Nov 3, 2011
Member

Any reason to keep those comments here, or is it a merge artifact?

This comment has been minimized.

@edavis10

edavis10 Nov 3, 2011
Author Contributor

I"ll remove them.

They were from when this was a plugin to keep track of what changes I made. The lines in this file were too dense to rely on git for it alone.


<li class="move"><%= context_menu_link l(:button_move), {:controller => 'issues', :action => 'move', :ids => @issues.collect(&:id)},
:class => 'context_item', :disabled => !@can[:move] %></li>
<%# All users can copy because the copy acton will check their permissions on the target project %>

This comment has been minimized.

@thegcat

thegcat Nov 3, 2011
Member

Again: Comments in templates?

This comment has been minimized.

@edavis10

edavis10 Nov 3, 2011
Author Contributor

I'll remove the comment.

Same reason as above (documentation while it was a plugin).

<%= link_to_if_authorized l(:button_log_time), {:controller => 'timelog', :action => 'new', :issue_id => @issue}, :class => 'icon icon-time-add' %>
<%= watcher_link(@issue, User.current, {:class => 'watcher_link', :replace => ['#watchers', '.watcher_link']}) %>
<%= link_to_if_authorized l(:button_duplicate), {:controller => 'issues', :action => 'new', :project_id => @project, :copy_from => @issue }, :class => 'icon icon-duplicate' %>
<%= link_to_if_authorized l(:button_copy), {:controller => 'issue_moves', :action => 'new', :id => @issue, :copy_options => {:copy => 't'}}, :class => 'icon icon-copy' %>
<%= link_to_if_authorized l(:button_move), {:controller => 'issue_moves', :action => 'new', :id => @issue}, :class => 'icon icon-move' %>
<%= link_to_if_authorized l(:button_delete), {:controller => 'issues', :action => 'destroy', :id => @issue}, :confirm => (@issue.leaf? ? l(:text_are_you_sure) : l(:text_are_you_sure_with_children)), :method => :post, :class => 'icon icon-del' %>
<div class="update button-large">

This comment has been minimized.

@thegcat

thegcat Nov 3, 2011
Member

.button-large is not a semantic but a "design" class, shouldn't be solved like this.

This comment has been minimized.

@edavis10

edavis10 Nov 3, 2011
Author Contributor

I'll change it.

This comment has been minimized.

@edavis10

edavis10 Nov 6, 2011
Author Contributor

After thinking about it, 'button large' still sounds like the best fit. The HTML is saying there should be a large button here but isn't saying what that design should be.

I'm leaving this alone for now. If you have a better name we can change it later.

@@ -12,7 +12,7 @@
<% end %>
</tr></thead>
<% previous_group = false %>
<tbody>
<tbody id="issue-list-body">

This comment has been minimized.

@thegcat

thegcat Nov 3, 2011
Member

These 2 ids will cause problems if the partial is reused somewhere else and don't provide more info than already there (besides shorter selectors).

This comment has been minimized.

@edavis10

edavis10 Nov 3, 2011
Author Contributor

I'll change it.

This comment has been minimized.

@edavis10

edavis10 Nov 6, 2011
Author Contributor

Removed the issue-list-body and issue-header ids. I think they were added for a feature I removed (lightbox form for a new issue).

@@ -1,10 +1,12 @@
<div class="contextual">
<% if authorize_for('issue_relations', 'new') %>
<%= toggle_link l(:button_add), 'new-relation-form', {:focus => 'relation_issue_to_id'} %>
<% end %>
</div>

This comment has been minimized.

@thegcat

thegcat Nov 3, 2011
Member

Any reason to keep the empty div.contextual? Also: Subtasks still have the "add" link on the right (in a div.contextual), that's very confusing.

Furthermore: Just having "Add" is probably not explicit enough, any ideas for a better text/link/icon. Maybe something similar to the "+" icon next to the categories/versions dropdown menus?

This comment has been minimized.

@edavis10

edavis10 Nov 3, 2011
Author Contributor

I'll have to see what would happen by removing it.

Subtasks should be changed too, the design was pre-subtasks.

This comment has been minimized.

@edavis10

edavis10 Nov 6, 2011
Author Contributor

Removing the contextual divs is fine. I also updated the subtasks "Add" link to match.

<!--[if lte IE 7]><%= stylesheet_link_tag 'ie7', :media => 'all' %><![endif]-->
<!--[if gte IE 8]><![endif]-->

<%= javascript_include_tag 'jquery.1.3.2.min.js' %>

This comment has been minimized.

@thegcat

thegcat Nov 3, 2011
Member

Any reason preventing us from using a current jquery?

This comment has been minimized.

@edavis10

edavis10 Nov 3, 2011
Author Contributor

The design has been tested with this version only. I have an issue to upgrade to a newer version that I'll work on after this is merged.

<%= stylesheet_link_tag 'rtl', :media => 'all' if l(:direction) == 'rtl' %>
<!--[if lte IE 6]><%= stylesheet_link_tag 'ie6', :media => 'all' %><![endif]-->
<!--[if lte IE 7]><%= stylesheet_link_tag 'ie7', :media => 'all' %><![endif]-->
<!--[if gte IE 8]><![endif]-->

This comment has been minimized.

@thegcat

thegcat Nov 3, 2011
Member

Would it be OK for you change those to selectors on the body (body.ie6, body.ie7 for the css and something like <!--[if lte IE 6]><body class="ie ie6"><![endif]--> in the page? That way plugins could add specific ie rules in their css, and the extra (core) css files can be merged to the theme too.

This comment has been minimized.

@edavis10

edavis10 Nov 3, 2011
Author Contributor

That doesn't make sense to me. These are done so each version of IE gets only the files it needs. What you're proposing wouldn't work because:

  • the body element becomes a case statement (case when ie7 <body.ie7>; when ie6 <body.ie6>; else <body>)
  • non-IE browsers would skip over if-comments, so they wouldn't have a body at all
  • you can't put a default body outside of the if-comments because then ie would get two bodies

I'll have to see the code you are proposing if this isn't what you meant.

This comment has been minimized.

@meineerde

meineerde Nov 3, 2011
Member

This is how I use it on http://meine-er.de which produces correct syntax for all browsers:

<!--[if lt IE 7 ]> <body class="ie ie6"> <![endif]-->
<!--[if IE 7 ]> <body class="ie ie7"> <![endif]-->
<!--[if IE 8 ]> <body class="ie ie8"> <![endif]-->
<!--[if IE 9 ]> <body class="ie ie9"> <![endif]-->
<!--[if (gt IE 9)|!(IE)]><!--> <body class='non-ie'> <!--<![endif]-->

This comment has been minimized.

@edavis10

edavis10 Nov 3, 2011
Author Contributor

No offensive but that looks horrible, especially the last case. And each of these needs to run through erb because our body looks like <body class="<%=h body_css_classes %>">.

This comment has been minimized.

@meineerde

meineerde Nov 3, 2011
Member

The text is inserted verbatim into the layout. The body_base_classes can be inserted into each of the optional body tags. So ERB has no additional overhead.

The upside is that it don't require to load an additional stylesheet which has to be maintained separately and thus looses locality. It would also break once we use the assets pipeline in Rails 3.1.

This exact approach is also taken by the HTML5 Boilerplate which I consider best practice. (Well, they updated it to set the classes on the <html> tag, but still...)

This comment has been minimized.

@edavis10

edavis10 Nov 3, 2011
Author Contributor

The text is inserted verbatim into the layout. The body_base_classes can be inserted into each of the optional body tags. So ERB has no additional overhead.

There is an overhead because erb would run 5 times and before it's sent to the browser. And then we would have to make sure to change the body tag 5 times if it changes.

<!--[if lt IE 7 ]> <body class="ie ie6 <%=h body_css_classes %>"> <![endif]-->
<!--[if IE 7 ]> <body class="ie ie7 <%=h body_css_classes %>"> <![endif]-->
<!--[if IE 8 ]> <body class="ie ie8 <%=h body_css_classes %>"> <![endif]-->
<!--[if IE 9 ]> <body class="ie ie9 <%=h body_css_classes %>"> <![endif]-->
<!--[if (gt IE 9)|!(IE)]><!--> <body class='non-ie <%=h body_css_classes %>'> <!--<![endif]-->

The upside is that it don't require to load an additional stylesheet which has to be maintained separately and thus looses locality.

Loading an additional stylesheet isn't that much, especially once they are cached in the client cache.

It would also break once we use the assets pipeline in Rails 3.1.

That discussion should wait until we are on Rails 3.1 and using the assets pipeline.

This exact approach is also taken by the HTML5 Boilerplate which I consider best practice. (Well, they updated it to set the classes on the tag, but still...)

I don't consider that best practice yet. Including IE specific stylesheets has been the "best practice" for years now. If you look at our current layout that is what is done for IE6 support right now (using an inline stylesheet).

<% display_sidebar = true %>
<% else %>
<% display_sidebar = false %>
<% end %>

This comment has been minimized.

@thegcat

thegcat Nov 3, 2011
Member

This should probably go in a helper, it at all.

This comment has been minimized.

@edavis10

edavis10 Nov 3, 2011
Author Contributor

I was waiting to refactor this.

@@ -2,7 +2,7 @@
<%= auto_discovery_link_tag(:atom, {:action => 'index', :format => 'atom', :key => User.current.rss_key}) %>
<% end %>

<div class="contextual">
<div id="project-links"class="contextual">

This comment has been minimized.

@thegcat

thegcat Nov 3, 2011
Member

(Misses a space)

@@ -11,6 +11,8 @@

<h2><%=l(:label_project_plural)%></h2>

<%= textilizable Setting.welcome_text %>

This comment has been minimized.

@thegcat

thegcat Nov 3, 2011
Member

Misses a div.wiki around it to work. Did you plan to replace the welcome page with the project list, or why has this landed here?

This comment has been minimized.

@edavis10

edavis10 Nov 3, 2011
Author Contributor

I'll remove this.

It was intended as an introduction to the users but I think it's out of place.

@@ -0,0 +1,5 @@
<% if @project %>
<div class="new-issue button-large">

This comment has been minimized.

@thegcat

thegcat Nov 3, 2011
Member

(see above comment about .button-large)

<h3><%= l(:label_version_plural) %></h3>
<% @versions.each do |version| %>
<%= link_to format_version_name(version), "##{version.name}" %><br />
<% end %>

This comment has been minimized.

@thegcat

thegcat Nov 3, 2011
Member

This should go into a submenu of the "Roadmap".

This comment has been minimized.

@edavis10

edavis10 Nov 3, 2011
Author Contributor

I'll revert this.

Submenus are a separate issue because some discussion needs to happen with them first.

@thegcat
Copy link
Member

@thegcat thegcat commented Nov 3, 2011

public/images/pencil.png.oxygen is probably a source file that shouldn't still be there.

@@ -0,0 +1,13 @@
/**
* DD_belatedPNG: Adds IE6 support: PNG images for CSS background-image and HTML <IMG/>.

This comment has been minimized.

@thegcat

thegcat Nov 3, 2011
Member

Is this used somewhere?

This comment has been minimized.

@edavis10

edavis10 Nov 3, 2011
Author Contributor

No, I thought I removed all of the references but I missed this one.

@@ -19,59 +19,69 @@ h4, .wiki h3 {font-size: 13px;padding: 2px 10px 1px 0px;margin-bottom: 5px; bord
padding: 0px 0px 0px 0px;
white-space:nowrap;
}
#top-menu a {color: #fff; margin-right: 8px; font-weight: bold;}
#top-menu a {color: #444;; margin-right: 0px; font-weight: bold;}

This comment has been minimized.

@thegcat

thegcat Nov 3, 2011
Member

s/;;/;/

else
params[:controller]
end

This comment has been minimized.

@thegcat

thegcat Nov 3, 2011
Member

That should definitely not be here. The cleanest current way to achieve "exceptions" for which menu item to open/highlight probably is an extension of the Redmine::MenuManager::MenuController.menu_item.

This comment has been minimized.

@edavis10

edavis10 Nov 3, 2011
Author Contributor

Maybe but that is left in until we figure out submenus: https://www.chiliproject.org/issues/559#note-4.

@@ -26,24 +26,29 @@ module JournalsHelper

def render_journal(model, journal, options = {})
return "" if journal.initial?
journal_content = render_journal_details(journal, :label_updated_time_by)
journal_content += render_notes(model, journal, options) unless journal.notes.blank?
journal_content = render_journal_details(journal, :label_updated_time_by, model, options)

This comment has been minimized.

@thegcat

thegcat Nov 3, 2011
Member

We lose flexibility by including render_notes into render_journal_details. What about extracting the header string without the call to render_notes into a render_journal_header method, so that render_journal is render_journal_header; render_notes; render_jounral_details?

This comment has been minimized.

@edavis10

edavis10 Nov 3, 2011
Author Contributor

Maybe, see my global note below about refactoring.

@edavis10
Copy link
Contributor Author

@edavis10 edavis10 commented Nov 3, 2011

I'm not sure if I made this clear yet.

This pull request is to get the first version of the redesign in place. Yes there might be visual bugs, yes there are places where we can refactor things to be cleaner. But if we block this pull request and wait until it's "perfect" then we might as well wait 12 months before the redesign is done.

Having something "good" now is 100x better than waiting months until it's "perfect". Especially since the next stage is already blocked and waiting (finn-design).

@edavis10 edavis10 merged commit 7bc11f8 into chiliproject:unstable Nov 7, 2011
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

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