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

aside #27

Merged
merged 2 commits into from
Nov 12, 2012
Merged

aside #27

merged 2 commits into from
Nov 12, 2012

Conversation

bryanlarsen
Copy link
Member

hobo_clean defines content like this:

    <section param="content"/>

hobo_bootstrap defines it like this:

    <div param="container">
      <div class="row" param="main-row">
        <div param="main-column">
          <section param="content"/>
        </div>
        <div param="aside-column">
          <div class="well">
            <section param="aside"/>
          </div>
        </div>
      </div>
    </div>

the hobo generators use the above like this:

<content: param>
  <section-group>
    <section param="main-content">
      <header param="content-header"> ... </header>
      <section param="content-body">  ... </section>
    </section>
    <aside param>  ... </aside>
  </section-group>
</content:>

Notice that the generated code does not use the aside param from hobo_bootstrap. So the aside ends up being a section at the bottom of the page.

@bryanlarsen
Copy link
Member Author

Solution #1: Hack up hobo_bootstrap and the aside definition

<def tag="aside" attrs="empty">
  <% @_aside_content = parameters.default %>
</def>

    <%
      # the aside tag sets @_aside_content, may be used in parameters.content
      # so evaluate content now, which may populate @_aside_content
      content=parameters.content[1][:default].call(nil)
      if !@_aside_content && !parameters.aside.blank?
        @_aside_content=parameters.aside[1][:default].call(nil)
      end
      if @_aside_content
        attributes[:content_size] ||= "9"
        attributes[:aside_size] ||= (12 - attributes[:content_size].to_i).to_s
        aside_span = "span" + attributes[:aside_size]
        aside_location ||= 'right'
      else
        attributes[:content_size] = "12" unless attributes[:content_size]
      end
    %>
    <div class="container bootstrap-content" param="container">
      <div class="row" param="main-row">
        <div class="#{aside_span}" if="&@_aside_content && aside_location=='left'" param="aside-column">
          <div class="well">
            <section class="aside"><%= @_aside_content %></section>
          </div>
        </div>
        <div class="span#{attributes[:content_size]}" param="main-column">
          <section with-flash-messages class="content"><%= content %></section>
        </div>
        <div class="#{aside_span}" if="&@_aside_content && aside_location=='right'" param="aside-column">
          <div class="well">
            <section class="aside"><%= @_aside_content %></section>
          </div>
        </div>
      </div>
    </div>

This works, but it's damn ugly, and breaks pseudo-parameters like after-content. (which is used in one of our integration tests).

@bryanlarsen
Copy link
Member Author

Solution #2: modify the hobo generators to use the content-body & aside parameters instead, and require all themes to define them.

I'm leaning towards this solution ATM. Thoughts?

@owendall
Copy link
Member

owendall commented Nov 9, 2012

+1 from me.

@iox
Copy link
Member

iox commented Nov 9, 2012

I like solution 2 too :).

@iox
Copy link
Member

iox commented Nov 10, 2012

The code looks good, but I'm unsure whether you have finished your work. Should I test and merge this pull request?

@bryanlarsen
Copy link
Member Author

Please test it. I'm going to add a version bump to both pull requests so because old hobo_bootstrap won't work with new hobo and vice versa. Then we just have to make sure that we merge the pull requests into both hobo & hobo_bootstrap simultaneously.

@iox
Copy link
Member

iox commented Nov 12, 2012

I tested both branches (hobo_bootstrap#aside and hobo#issue_11) and they work very nicely :). Please merge when you want and I'll push the gem.

By the way, I took a screenshot of the aside and found a bug in the process: #30

@bryanlarsen bryanlarsen merged commit 695598a into master Nov 12, 2012
@bryanlarsen
Copy link
Member Author

Merged. Don't push the gem yet. People using hobo from github should be
using hobo_bootstrap from github, too.

Bryan

On Mon, Nov 12, 2012 at 2:07 PM, Ignacio Huerta [email protected]:

I tested both branches (hobo_bootstrap#aside and hobo#issue_11) and they
work very nicely :). Please merge when you want and I'll push the gem.

By the way, I took a screenshot of the aside and found a bug in the
process: #30 #30


Reply to this email directly or view it on GitHubhttps://github.com//pull/27#issuecomment-10299929.

@iox
Copy link
Member

iox commented Nov 13, 2012

Ok :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants