cloudfour / drizzle

A streamlined tool for developing, documenting and presenting UI pattern libraries.
https://cloudfour.com/thinks/introducing-drizzle/
MIT License
666 stars 32 forks source link

DRY labels #26

Closed gerardo-rodriguez closed 8 years ago

gerardo-rodriguez commented 8 years ago

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

erikjung commented 8 years ago

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 commented 8 years ago

@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 commented 8 years ago

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

gerardo-rodriguez commented 8 years ago

@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 commented 8 years ago

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

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 commented 8 years ago

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

gerardo-rodriguez commented 8 years ago

@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 commented 8 years ago

@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 commented 8 years ago

@mrgerardorodriguez It's a beauty! 👍

tylersticka commented 8 years ago

LGTM 👍