piotrmurach / loaf

Manages and displays breadcrumb trails in Rails app - lean & mean.
MIT License
406 stars 24 forks source link

Add helper & controller method #breadcrumb_names, with tests #42

Closed pond closed 3 years ago

pond commented 4 years ago

Describe the change

This adds helper breadcrumb_names for views and corresponding method for controllers.

Why are we doing this?

Our site makes use of Loaf for breadcrumbs, but prior to this change didn't have any way to reuse that data for title attributes in the HTML. Using _breadcrumbs.last.name was helpful here, but we wanted to make this an 'official' public API rather than using the internal stuff.

Benefits

Other users can apply similar changes, or use the name(s) array in ways we haven't considered.

Drawbacks

Bloats the API by one method call.

Requirements

Put an X between brackets on each line if you have done the item: [x] Tests written & passing locally? [x] Code style checked? [x] Rebased with master branch? [x] Documentation updated?

pond commented 4 years ago

I note Hound CI's complaints in the test additions, but as far as I can see I'm just copying the coding style (e.g. single quoted strings and indentation alignment of "=" etc.) therein. I can change everything to eliminate the warnings if you prefer.

coveralls commented 4 years ago

Coverage Status

Coverage increased (+0.08%) to 97.03% when pulling 4fce9d1dbf666a898de11229208e1b4a9f73b4ab on RIPGlobal:feature/public-access-to-crumbs into 6a8d586d3a49b4e524be425dee86d5def8eb79f1 on piotrmurach:master.

piotrmurach commented 4 years ago

Thank you so much for this PR! I reviewed the changes and they seem reasonable. However, as a maintainer, I need to carefully consider changes/extensions to the current API. I like to keep things as minimal as possible.

As you mentioned in the drawbacks section this adds new API methods. The question is whether this library needs the new extension points. This gem is built predominantly for breadcrumbs rendering and using it for titles is a nice 'side effect' but not the core focus. Equally, the breadcrumb_trail returns an enumerator and it should be possible to do the following in a view:

breadcrumb_trail.map(&:name)

As for having this functionality in the controllers, I'm very sceptical of its utility. What do you reckon? Would changes to documentation suffice?

dsazup commented 3 years ago

Hey both.

As a user of this library, I can see how this change makes it a little bit nicer to use. However, if I was a maintainer of this library - I would just advise to put this code in the ApplicationHelper, simply because it adds additional 75 lines of code, while it only saves 10 characters.

Even with this change I would still need to do some work, such as join the array with a symbol like Home > Library > Books, right? So personally I would just have something like this in my application helper and call it a day 🤷‍♂️

module ApplicationHelper
  def breadcrumbs_for_title
    breadcrumb_trail.map(&:name).join(">")
  end
end
piotrmurach commented 3 years ago

@dsazup Thanks! I added your example to the README.

@pond Having consider this PR carefully, I won't be merging the changes. I added documentation on how the breadcrumb names can be retrieved by chaining returned enumerator. I consider it a good compromise. It also turns out that having breadcrumb_names helper skips the translation lookup for titles and hence wouldn't be backwards compatible. Thank you for submitting this PR.

pond commented 3 years ago

Ultimately I was after an "official" way to get at something for a title and the docs now show a good way to do it, so that seems fine to me.

Appreciate all the consideration and conversation from the contributors herein. Thanks!

piotrmurach commented 3 years ago

@pond Thanks for your understanding. Released a new v0.10.0 version that should be ready for the Rails 6.1 release.