RFC: Yield link-to component state

[sukima]

Rendered

[sukima]

Related Implementation

[jamesarosen]

I strongly support the overall concept, but "active" already has another meaning for <a> tags. I might recommend link.current or link.isCurrent to map to current-when.

[sandstrom]

Positive to this RFC! 🏅 (or some other method which would relieve us of our current hack, quoted below)

We currently need a quite complicated hack (below) to toggle a class on a parent element to the link element, depending on the state of the link.

This is common in menus, where the parent element of the active link should be styled differently.

Seems like it should remove the hack we're currently using to solve this issue. I've posted the hack below, as an example of how complicated this currently is.

templates/something.hbs

{{#active-link-wrapper class='large' as |alw|}}
  {{#link-to 'myRoute' wrapper=alw}}my link text{{/link-to}}
{{/active-link-wrapper}}

components/active-link-wrapper.js

import Ember from 'ember';
import { retrieveEventPath } from 'client/utils/dom';

export default Ember.Component.extend({

  // Used to enlarge the click-area of links (useful for touch-devices),
  // and allows the 'active' class to exist on a parent element.
  //
  // NOTE
  // Related code is in 'reopens/link-component'.

  tagName: 'span',

  classNameBindings: ['active'],
  classNames: ['active-link-wrapper'],

  active: function() {
    return this.get('_linksList').isAny('active');
  }.property('_linksList.@each.active'),

  init() {
    this._super();
    this.set('_linksList', Ember.A());
  },

  // Act only if the target is this wrapper/element or a non-link element, do nothing when targeting the link directly.
  // Otherwise we're creating an additional synthetic click.
  click(jqEvent) {
    let event = jqEvent.originalEvent;

    // slice out elements between event target and this component element, and see if any of them is a link
    let eventPath = Array.from(retrieveEventPath(event)); // cast NodeList to Array
    let linkInPath = eventPath.slice(0, eventPath.indexOf(this.get('element'))).some((e) => e.matches('a'));

    if ((event.target === this.get('element')) || !linkInPath) {
      let l = this.get('_linksList')[0];

      if (l) {
        l._invoke(event);
      }
    }
  },

  // Scheduling needed to avoid `You modified ${label} twice in a single render` deprecation message.
  // This stuff is hacky to begin with, and will be replaced when a better option is available.
  registerLink(link) {
    Ember.run.scheduleOnce('afterRender', this, function() {
      this.get('_linksList').addObject(link);
    });
  },

  deregisterLink(link) {
    Ember.run.scheduleOnce('afterRender', this, function() {
      this.get('_linksList').removeObject(link);
    });
  },

});

link-component-reopen.js

Ember.LinkComponent.reopen({

    // Communication with ActiveLinkWrapper
    //
    // Need to know of the child link that it wraps.
    // Using a computed property setter trick to register each link with the wrapper.
    // (there is no easy way to 'subclass' the link-helper, if one existed this could be less hacky)
    //
    // https://github.com/emberjs/ember.js/issues/10573
    // https://github.com/emberjs/ember.js/blob/680f997e/packages/ember-routing-htmlbars/lib/keywords/link-to.js

    _activeLinkWrapper: null,

    wrapper: Ember.computed({
      get(_key) {
        return this.get('_activeLinkWrapper');
      },
      set(key, value) {
        if (value) {
          value.registerLink(this); // value is an instance of `ActiveLinkWrapper`
          this.set('_activeLinkWrapper', value);
          return value;
        }
      },
    }),

    willDestroyElement() {
      this._super();

      let alw = this.get('_activeLinkWrapper');
      if (alw) {
        alw.deregisterLink(this);
      }
    },

  });

Click 'details' to see our current hack for links and nested elements.

[locks]

We have a Guide dedicated to link-to, where it would likely be warranted for the API in this RFC to be covered.

[wycats]

@sukima are you planning to update the RFC to replace the boilerplate from the template with some content for those sections?

My personal feedback is:

• Instead of overloading {{#link-to, we should pick a new name.
• The RFC should describe what would be needed to implement today's link-to functionality in terms of this new helper.
• In particular, does <a "just work" or is additional work needed to ensure that interacting with the link is correctly intercepted.

[wycats]

@MelSumner What kind of accessibility considerations are relevant for JavaScript-enhanced links?

[Panman82]

Instead of overloading {{#link-to, we should pick a new name.

@wycats but wouldn't that cause even more API surface by duplicating everything {{link-to}} just to also allow the yielded state? Or are you thinking a completely different API for this, perhaps in transition to something new? Personally, I'd rather see this added directly to {{link-to}}. It "just makes sense" and adds upon what was enabled from the Router Service RFC.

@jamesarosen isActive is already used in the Router Service, so IMO this should stick with that name for consistency.

@sandstrom Your solution has also been available as an ember addon, ember-cli-active-link-wrapper. But I don't think this replaces that solution because {{#active-link}} doesn't create a clickable link itself, it just looks for nested active links to apply the active class to the parent.

[sandstrom]

@Panman8201 True, but if this RFC were to be implemented, we could use something like this, and we'd get the active-class on both the li and the a.

{{#link-to "my-route" tagName="li" as |link|}}
  <a href={{link.href}}>My Route</a>
{{/link-to}}

The only difference is that the li would also be clickable (but that wouldn't be an issue for us).

[Panman82]

@sandstrom Ah, yeah, that would be a good fit. Guess I was thinking in the case of navbar dropdowns, where one of the many nested links could be active and the parent/container also needs the active state.

[wycats]

@Panman8201 the issue with tacking more things onto {{link-to}} is that {{link-to}} as it currently stands has no choice but to opt you into a bunch of mandatory behavior (attributes that have to be computed and applied to the tag unconditionally in case CSS is using them, etc.) that make things slow.

Your API design doesn't have that problem, because it yields information into the block that you are supposed to use on your own, but it looks darn similar to the existing API, completely altering the behavior purely based on the block's arity. Importantly, the improvements to the API surface in your proposal also mean that the implementation shouldn't be shared.

In general, we haven't swapped semantics and implementation in Glimmer templates based purely on the arity of the block, and I don't really want to start now 😄

[knownasilya]

This could be done in addon space first, using RouterService#urlFor. Something along the lines of the following:

{{#url-for 'my.route' some.id as |url isActive|}}
  <a href={{url}} class={{if isActive 'active'}}>{{name}}</a>
{{/url-for}}

{{!-- or a helper version, with active done manually via the service --}}

<a href={{url-for 'my.route' some.id}} class={{if isActive 'active'}}>{{name}}</a>

[sukima]

Based on @knownasilya's suggestion should we consider this RFC closed (unmerged) as the new API/implementation is best suited for an addon?

[wycats]

@sukima I think it would be awesome for this to start out as an addon, but once it gets going, let's revisit this RFC. I really do think it makes sense for us to reform link-to, but the point of the Routing Service RFC was to allow people to try things out as addons first, and I think that's right here.