Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

DOM-less active link wrapper #22

Closed
wants to merge 4 commits into from

Conversation

billdami
Copy link

This PR is my attempt at refactoring this addon so that it is fully compatible with FastBoot. While the addon does now play nicely with fastboot after moving buildChildLinkView to the didInsertElement hook (which is not executed on the server), it did not give us full rendering parity between the server and client. This results in a bit of an undesirable side effect where the link wrappers initially render in the browser as not active, and then the active state "pops in" once the client-side ember app loads and takes over.

To solve this, instead of relying on jQuery or the DOM in general being available to query for matching link elements, I refactored this into 2 co-dependent components active-link (which is the link itself now, and merely extends Ember.LinkComponent), and active-link-container (which is the wrapper <li> element). The child active-link components now walk up the component tree looking for ancestor active-link-container components that they register themselves to, which is how the childLinkViews array is built.

@alexspeller
Copy link
Owner

Thanks for the PR but I don't think I would want to follow the approach - it means that the component is no longer a drop-in use and you'd have to change all the link-to elements.

If this approach was to be followed, it would be easier to use contextual components instead of walking up the tree to find parents, and that would enable using entirely public api.

@billdami
Copy link
Author

billdami commented Oct 15, 2016

Thanks for the PR but I don't think I would want to follow the approach - it means that the component is no longer a drop-in use and you'd have to change all the link-to elements.

@alexspeller Yeah I understand, I figured there was a good chance you wouldn't want to go in this direction as it would completely change the API of the addon, it was partly more of a way for me to just show an alternate approach that was 100% fastboot-friendly and get some eyes and feedback on it at the very least.

If this approach was to be followed, it would be easier to use contextual components instead of walking up the tree to find parents, and that would enable using entirely public api.

Thats an interesting idea, although wouldn't that not work for the case of nested active link wrappers (where you want the parent wrapper to be active if all sub links are active), since the yielded component would only have a reference to its immediate parent? E.g.

{{#active-link-container as |active-link|}}
  {{#active-link}}Parent Link{{/active-link}}
  <ul>
    {{#active-link-container as |active-link|}}
      {{#active-link}}Child Link{{/active-link}}
    {{/active-link}}
  </ul>
{{/active-link-container}}

@alexspeller
Copy link
Owner

The api could look like this I think:

{{#active-link-container as |active|}}
  {{#active.link}}Parent Link{{/active.link}}
  <ul>
    {{#active.link-container as |nestedActive|}}
      {{#nestedActive.link}}Child Link{{/nestedActive.link}}
    {{/active.link-container}}
  </ul>
{{/active-link-container}}

I.e. by yielding both a link component and a contianer component in the hash, it's possible to handle a nested case if you need to

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants