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

[BUGFIX beta] Make link-to a real component. #12176

Merged
merged 1 commit into from
Aug 22, 2015

Conversation

nathanhammond
Copy link
Member

Alright, I commit to bringing this home. To-do list from #11962 plus one more:

  • Use the new arbitrary-length positional params feature.
  • Expose hasBlock on components, privately.
  • Use a nested helper to avoid exposing escaped.
  • Performance review.
  • Test

// now that we have the component instance.
layout = get(component, 'layout') || layout;
component._hasBlock = !!templates.default;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This should be provided as an option in createOptions (so that it is available at init and doesn't change the shape of the object after initialization).

@rwjblue
Copy link
Member

rwjblue commented Aug 22, 2015

We chatted a little bit about this in slack this evening, I think we have a path forward to allowing simple extending of Ember.LinkComponent without exposing hasBlock to the JS-world carte-blanche (via a symbol).


// TODO: Remove once `hasBlock` is working again
attrs.hasBlock = !!template;
Ember.runInDebug(() => {
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this kosher? This is only used for an error message inside an Ember.Deprecate call.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ya, seems fine. Can you also underscore (so hash._view)?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is our build story clever enough to drop an import if it's only used inside of a runInDebug? We could move this to a symbol and stop squatting on properties.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I believe the import would still exist, but I think that is likely fine. Using a symbol here seems good (and helps prevent folks that we are going to start encouraging to use Ember.LinkComponent.extend from relying on the private internals...).

@@ -0,0 +1,5 @@
import { symbol } from 'ember-metal/utils';
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This doesn't seem like a compat type thing to me, maybe we can move it to packages/ember-views/lib/system/ instead?

@nathanhammond nathanhammond changed the title [WIP] link-to as real component [BUGFIX beta] link-to as real component Aug 22, 2015
@nathanhammond
Copy link
Member Author

I went with [BUGFIX beta] as this changes nothing and closes #11962.

import { symbol } from 'ember-metal/utils';

// These symbols will be used to limit link-to's public API surface area.
let HAS_BLOCK = symbol('HAS_BLOCK');
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

These can be export let HAS_BLOCK... (instead of separate declaration and export statements).

@rwjblue
Copy link
Member

rwjblue commented Aug 22, 2015

I went with [BUGFIX beta]

👍

@rwjblue
Copy link
Member

rwjblue commented Aug 22, 2015

Can you also add [BUGFIX beta] as a commit prefix (I see that you updated the PR title, but not the commit itself)?

@nathanhammond nathanhammond changed the title [BUGFIX beta] link-to as real component [BUGFIX beta] Make link-to a real component. Aug 22, 2015
@rwjblue
Copy link
Member

rwjblue commented Aug 22, 2015

:shipit:

rwjblue added a commit that referenced this pull request Aug 22, 2015
[BUGFIX beta] Make link-to a real component.
@rwjblue rwjblue merged commit 849bec1 into emberjs:master Aug 22, 2015
@rwjblue rwjblue mentioned this pull request Aug 22, 2015
3 tasks
@mmun
Copy link
Member

mmun commented Aug 22, 2015

Good work. Hopefully with @ef4's strategy we can drop the HAS_BLOCK symbol entirely.

@nathanhammond
Copy link
Member Author

Once we have "splat" attrs I think we can adopt @ef4's approach: master...ef4:link-to-cleanup

(/at this point I know it pretty well and might just pick up where he left off.)

@nathanhammond nathanhammond deleted the link-to-component branch August 22, 2015 20:19
@mmun
Copy link
Member

mmun commented Aug 22, 2015

I think you can just copy the attrs onto the <a> tag in didRender? Splat would be better tho. :)

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