Windvis / ember-breadcrumb-trail

Minimalistic but very flexible breadcrumb management solution for Ember applications.
MIT License
14 stars 3 forks source link

Provide a public way to access the breadcrumbs in JS code #9

Closed Windvis closed 3 years ago

Windvis commented 3 years ago

BreadcrumbsService.items is a private API at the moment. It might be nice to make it public API so that a consuming app can access it directly in helpers / components without having to pass it in.

I think it's a nice thing to have, but I'm not sure about the naming of the public property yet.

A theorectical is-last helper that returns true if the breadcrumb is the last item can have the following basic implementation:

import { helper } from '@ember/component/helper';

export default helper(function isLast([breadcrumb, breadcrumbs]) {
  return breadcrumbs[breadcrumbs.length - 1] === breadcrumb;
});

And used like this:

{{#each (breadcrumbs) as |breadcrumb|}}
  <li>
    {{#if (not (is-last breadcrumb (breadcrumbs))}}
      <LinkTo @route={{breadcrumb.data.route}}>
        {{breadcrumb.title}}
      </LinkTo>
    {{else}}
      {{breadcrumb.title}}
    {{/if}}
  </li>
{{/each}}

If BreadcrumbsService.items was public API the DX could be improved:

import Helper from '@ember/component/helper';
import { inject as service } from '@ember/service';

export default class IsLastHelper extends Helper {
  @service('breadcrumbs') breadcrumbsService;
  compute([breadcrumb]) {
    let breadcrumbs = this.breadcrumbsService.items;
    return breadcrumbs[breadcrumbs.length - 1] === breadcrumb;
  }
}

Usage:

{{#each (breadcrumbs) as |breadcrumb|}}
  <li>
    {{#if (not (is-last breadcrumb)}}
      <LinkTo @route={{breadcrumb.data.route}}>
        {{breadcrumb.title}}
      </LinkTo>
    {{else}}
      {{breadcrumb.title}}
    {{/if}}
  </li>
{{/each}}
cah-brian-gantzler commented 3 years ago

I was just about to write an issue in that the example code shown uses is-last and no implementation given on the readme. I see it here. If the above is done, I would suggest changing the is-last to is-last-crumb because is-last is way to generic a name for such a given specific purpose.

It may also be noted that https://github.com/DockYard/ember-composable-helpers#has-next is the not equivalent of the is-last and removes the need for the not in the above hbs.

items on the service may be private in theory, but there is no documentation notes on the service stating it is not public. I would suggest crumbs if you are looking for a name different than items.

I would also not re-export the service in the app directory and instead replace the service injections @service('EmberBreadcrumbTrail@breadcrumbs') breadcrumbsService; as this helps to avoid app namespace overrides

Windvis commented 3 years ago

I was just about to write an issue in that the example code shown uses is-last and no implementation given on the readme. I see it here. If the above is done, I would suggest changing the is-last to is-last-crumb because is-last is way to generic a name for such a given specific purpose.

I'm not planning on adding a helper like that to the addon. That's just an example of how this addon can be used. The docs mention it is a "custom" helper though, maybe I should change the wording to make that more clear?

Like you said, there are multiple ways to solve this already (and it's a generic problem as well). I actually think the {{each}} helper should expose the "is first" and "is last" data in some way (which it does in the vanilla Handlebars variant).

items on the service may be private in theory, but there is no documentation notes on the service stating it is not public. I would suggest crumbs if you are looking for a name different than items.

Exactly, the service isn't documented at all :smile: so it is considered private API. People who need it are going to use it anyway, even if explicitly marked private. I'm just waiting until I encounter valid use cases for this. The example in this issue isn't really a good one imo.

I would also not re-export the service in the app directory and instead replace the service injections @service('EmberBreadcrumbTrail@breadcrumbs') breadcrumbsService; as this helps to avoid app namespace overrides

Interesting, is that notation documented somewhere? The only reference I can find is this RFC which explicitly says it's going to be deprecated :smile:. But anyway, your reasoning is that not reexporting it like makes it possible for apps to create a "breadcrumbs" service themselves? I don't think addons commonly tend to avoid that? "breadcrumbs" is also kind of specific, I doubt anyone will do that if they use this addon. I'll keep it in mind though.

Windvis commented 3 years ago

Closing this as my example isn't the greatest and people who need this can open a new issue with their use case.

cah-brian-gantzler commented 3 years ago

Its is not documented as far as I can tell, but I found an addon using it, then I have put it in some other addons. Here is a PR in which it was added and the discussion involved rjwblue who confirmed its usage and availability for a surprising long time. Still dont know why it isnt documented. The RFC you posted says the current syntax is going to be deprecated, it is still going to be available, just the namespace (part before the @) will be a separate string.

Ember-yeti-table uses it at my suggestion, notice that only the main component is added to app, the sub components are not. I thought ember-bootrap was going to use it in some fashion but see they have not yet.

The use of namespace in components as <EmberStargate@Portal> serves as a great start to template imports.