windvis / ember-breadcrumb-trail Goto Github PK

Possible duplicate of #18

There is a bug causing the breadcrumb helper to not recompute when its dependant data changes. It only seems to happen when the breadcrumb item is somewhere in the middle of the breadcrumb stack (i.e. the breadcrumb in question has an active subroute).

So far the only way I can reproduce reliably is to use debugger to force a timing delay.

Reproduction: https://github.com/charlesfries/ember-breadcrumb-trail/tree/timing

Steps to reproduce:

1. Clone and serve.
2. Visit http://localhost:4200/foo/bar with the inspector open and breakpoints enabled
3. Skip the breakpoints until there are none

Expectation

The "Home" breadcrumb should say "Home hello" after a delay of 2000ms.

Actual

The "Home" breadcrumb says "Home".

"ember-breadcrumb-trail": "^0.2.0",
"ember-cli": "~3.28.0",
"ember-source": "~3.28.0",

Hey, first of all congrats for the addon! 😁
I like the declarative approach. Thinking about maybe adding a simple wrapper component to ember-bootstrap to auto-generate the appropriate Bootstrap markup based on what {{breadcrumbs}} returns...

Anyway, to the topic: I feel the addon could be a bit more opinionated about capturing the route parameters, as that is a use case you will always have. The router service exposes the current RouteInfo through currentRoute, which the {{breadcrumb}} helper could capture, and expose it by default just like title.

So your Readme's example could look something like this:

<nav aria-label="Breadcrumb">
  <ol>
    {{#each (breadcrumbs) as |breadcrumb|}}
      <li>
        <LinkTo
          @route={{breadcrumb.routeInfo.name}}
          @models={{breadcrumb.routeInfo.params}}
          @query{{breadcrumb.routeInfo.queryParams}}
          aria-current={{if (is-last breadcrumb breadcrumbs) "page"}}
        >
          {{breadcrumb.title}}
        </LinkTo>
      </li>
    {{/each}}
  </ol>
</nav>

{{! index.hbs }}
{{breadcrumb "Home"}}

{{! about.hbs }}
{{breadcrumb "About"}}

This would hopefully then work ootb also with more complicated routes, with dynamic segments/models and QPs. And it would actually allow my use case of a Bootstrap wrapper, as the routeInfo property becomes a public API that others could rely on, whereas otherwise I would need to force users into using my naming for those explicit arguments.

It's probably not quite as easy as it sounds here, because the {{breadcrumb}} helper would always see the leaf-most route, even the one that is on application.hbs. But the RouteInfo is traversable (.child/.parent), so maybe it's possible to match the stack of RouteInfos to the stack of breadcrumbs? That is assuming your helpers are used exactly in the way the routes are nested, if that's not the case (e.g. you skipped a helper invocation in a sub-route) then this approach probably breaks apart!? 🤔

Idk, just thinking aloud here, but that's thought I wanted to share quickly... 😉

The flags can be useful to style the first / last breadcrumb differently. Things I've seen where this can come in handy:

• Home icon before the first breadcrumb
• Make the last breadcrumb just plain text / non-interactive
• Add aria-current="page" to the last breadcrumb for a11y and SEO

Potential API:

{{#each (breadcrumbs) as |breadcrumb|}}
  <li>
    <a href={{breadcrumb.data.href}} aria-current={{if breadcrumb.isLast "page"}}>
      {{#if breadcrumb.isFirst}}
        <Icon @name="home" />
      {{/if}}
      {{breadcrumb.title}}
    </a>
  </li>
{{/each}}

Not sure if this needs to be implemented in the addon itself since this can already be accomplished through app code. Maybe it's enough to document how to do it.

Example using ember-truth-helpers and has-next from ember-composable-helpers:

{{#each (breadcrumbs) as |breadcrumb index|}}
  {{#let (eq index 0) (not (has-next breadcrumb breadcrumbs) as |isFirst isLast|}}
    <li>
      <a href={{breadcrumb.data.href}} aria-current={{if isLast "page"}}>
        {{#if isFirst}}
          <Icon @name="home" />
        {{/if}}
        {{breadcrumb.title}}
      </a>
    </li>
  {{/let}}
{{/each}}

It's also possible to add custom is-first / is-last helpers which makes this cleaner:

{{#each (breadcrumbs) as |breadcrumb|}}
  {{#let (is-first breadcrumb breadcrumbs) (is-last breadcrumb breadcrumbs) as |isFirst isLast|}}
    <li>
      <a href={{breadcrumb.data.href}} aria-current={{if isLast "page"}}>
        {{#if isFirst}}
          <Icon @name="home" />
        {{/if}}
        {{breadcrumb.title}}
      </a>
    </li>
  {{/let}}
{{/each}}

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}}

It seems we're still using ember-auto-import v1 internally so the Ember v4 jobs fail. Updating to the latest blueprints should solve that and is a good idea anyways.

The readme claims FastBoot support, but this isn't actually verified during CI.

https://github.com/embermap/ember-cli-fastboot-testing

The v2 addon format is the future of Ember addons so I would like to try it out on this one since it's small enough.

More information about the conversion: https://www.kaliber5.de/en/blog/v2-addon_en

The downside is that it will require ember-auto-import v2.

Sometimes going from a child route to a parent route has some weird timing problems.

Situation:
Overview / Details route with dynamic segment / sub page. The breadcrumbs bar is added to a top level route, so always visible.

Navigating between sub page and the details page is ok, but navigation to the parent page causes an issue. Ember tries to re-render the "Details" link with an undefined dynamic segment while it shouldn't need to render that link at all.

So it seems that the breadcrumb helper receives an update request before it's getting destroyed.

It might be worth it to cache the BreadcrumbsService.items result since it's potentially accessed multiple times by the user.

https://github.com/rwjblue/create-rwjblue-release-it-setup

I highly prefer template based breadcrumb solutions but the timing issue is a downside that requires workarounds. It might make sense to look for a Route based solution similar to ember-crumbly while keeping the flexibility of the current design.

A downside of this approach is that apps that don't use the model hooks to fetch data can't add breadcrumbs anymore.

ember-crumbly has an option to reverse the breadcrumbs. It seems to be useful for RTL languages but I have no experience with that.

This could potentially look like this:

{{#each (breadcrumbs reversed=true) as |breadcrumb|}}
  <li>
    {{breadcrumb.title}}
  </li>
{{/each}}

{{breadcrumb "foo"}}
{{breadcrumb "bar"}}

{{! bar < foo }}

Similar to #5 this can already be accomplished easily in the app itself. So I'm not sure if it needs to be part of the addon.

Example using ember-composable-helpers' reverse:

{{#each (reverse (breadcrumbs)) as |breadcrumb|}}
..
{{/each}}