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

DRY labels #26

Merged
merged 7 commits into from
Apr 26, 2016
Merged

DRY labels #26

merged 7 commits into from
Apr 26, 2016

Conversation

gerardo-rodriguez
Copy link
Member

This creates a partial for the labels. This addresses part of #22 (headings not addressed, there are enough differences between how they are used in page-item.hbs and item.hbs that I need to think that one through more).

cc: @erikjung @tylersticka

@@ -0,0 +1,11 @@
{{#if data.labels}}
Copy link
Contributor

Choose a reason for hiding this comment

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

I would keep the logic out of this partial and include in each parent if needed. The partial should assume that labels is present.

it can be called like:

{{#if data.labels}}
  {{> drizzle.labels labels=data.labels}}
{{/if}}

And then the partial can iterate over labels:

{{#each labels as |label|}}
...
{{/each}}

@erikjung
Copy link
Contributor

It might be too fragile, but I think we can address some of the heading use cases with additional logic in the template, for example:

<{{tag}} {{#if id}}id="{{id}}"{{/if}} class="
 drizzle-u-inlineBlock
 drizzle-u-alignMiddle
 drizzle-u-textNoWrap
 drizzle-u-padRight">
  {{#if href}}
    <a href="{{href}}" class="drizzle-u-linkHidden">
      {{text}}
    </a>
  {{else}}
    {{text}}
  {{/if}}
</{{tag}}>
{{> drizzle.heading tag="h3" id="foo" href="#foo" text="Heading"}}

OR, we could also leverage the layout helpers for something like this.

@gerardo-rodriguez
Copy link
Member Author

@erikjung Question: If I am using a two word name for the file, should it be labelheader.hbs or labelHeader.hbs or label-header.hbs? I see a page-item.hbs, but wasn't sure. Thanks!

@erikjung
Copy link
Contributor

erikjung commented Apr 26, 2016

@mrgerardorodriguez Out of those, the only awkward one is the camel-case IMO.

<a href="./{{key}}.html">{{title}}</a>
</h4>
{{/block}}
{{#if labels}}
Copy link
Member Author

Choose a reason for hiding this comment

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

@erikjung: I added this logic into this partial. What are your thoughts? Not all label headings will have labels.

@gerardo-rodriguez
Copy link
Member Author

@tylersticka @erikjung: Ready for a re-review, thanks! I added the heading to the labelheader.hbs partial. I then extended that for the 2nd variation labelheader-anchorid.hbs partial.

@erikjung
Copy link
Contributor

@mrgerardorodriguez I think ideally we should get this all wrapped into one partial, with logic to account for the variable things:

  • The heading tag (<h3>, <h4>, etc.)
  • Sometimes an <a> will be needed in the heading
  • Sometimes there will be labels to loop over

And with those conditions handled, the partial (assuming we keep the labelheader name) can be used in a way something like this:

{{#> drizzle.labelheader tag="h3" labels=data.labels  id=(toSlug @key)}}
  Heading Text
{{/drizzle.labelheader}}

It's a complex partial, but I think we should go all-in and make it do what we need, versus having to use multiple ones. I'd be happy to help out if needed.

@gerardo-rodriguez
Copy link
Member Author

Cool, thanks @erikjung! I'll give it a go and let you know if I need any help. 😉

drizzle-u-alignMiddle
drizzle-u-textNoWrap
drizzle-u-padRight">
{{#if key}}
Copy link
Member Author

Choose a reason for hiding this comment

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

@erikjung I'm not convinced by this if/else sequence...what are your thoughts?

Copy link
Contributor

Choose a reason for hiding this comment

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

@mrgerardorodriguez I think we can simplify it.

If we supply an id, then we can use that to determine whether or not the anchor is output.

{{#if id}}
  <a href="#{{id}}">{{> @partial-block}}</a>
{{else}}
  {{> @partial-block}}
{{/if}}
{{#> drizzle.labelheader id=(toSlug data.title)}}
  {{data.title}}
{{/drizzle.labelheader}}

Copy link
Member Author

Choose a reason for hiding this comment

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

Good catch, @erikjung. Just to be clear, there are still 3 possible outcomes.

  1. <a href="./{{key}}.html">{{> @partial-block}}</a>
  2. <a href="#{{id}}" class="drizzle-u-linkHidden">{{> @partial-block}}</a>
  3. {{> @partial-block}}

I'm not seeing an obvious way to more cleanly handle the differences between the first and the second.

Copy link
Contributor

Choose a reason for hiding this comment

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

@mrgerardorodriguez

Supplying an id lets us know to wrap things with a self-referencing "hidden" anchor. If we need to have an arbitrary <a> instead, we can omit the id and simply include whatever markup is needed via @partial-block:

{{#> drizzle.labelheader tag="h3"}}
  <a href="./{{@key}}.html">{{data.title}}</a>
{{/drizzle.labelheader}}

Copy link
Member Author

Choose a reason for hiding this comment

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

Ah, got it. Thanks for the advice @erikjung! 😀

@gerardo-rodriguez
Copy link
Member Author

@erikjung Okay, I refactored the both partials back into a single one.

Questions:

  1. Is it odd that I renamed the name key to title? Does something need to change in the drizzle-builder repo?
  2. The partial still feels clunky to me. How do you feel about it?

@gerardo-rodriguez
Copy link
Member Author

gerardo-rodriguez commented Apr 26, 2016

@erikjung @tylersticka: Ready for a re-review!

  1. Is it odd that I renamed the name key to title? Does something need to change in the drizzle-builder repo?

Feel free to ignore this, as this is no longer applicable.

  1. The partial still feels clunky to me. How do you feel about it?

Thanks for helping me work through this @erikjung! 😃

@erikjung
Copy link
Contributor

@mrgerardorodriguez It's a beauty! 👍

@tylersticka
Copy link
Member

LGTM 👍

@gerardo-rodriguez gerardo-rodriguez merged commit 9137586 into master Apr 26, 2016
@gerardo-rodriguez gerardo-rodriguez deleted the chore-dry_labels branch April 26, 2016 22:05
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