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

yield from the template #72

Merged
merged 1 commit into from Sep 2, 2015
Merged

yield from the template #72

merged 1 commit into from Sep 2, 2015

Conversation

davidgoli
Copy link
Collaborator

Previously, the only possible loading indicators were loadingText and a CSS background image. This gives a much bigger range of possibilities, such as CSS animations with custom markup.

@kellyselden
Copy link
Collaborator

#51. I believe this is already a feature, where you can swap in your own template.

@hhff
Copy link
Collaborator

hhff commented Aug 17, 2015

I think it would be nice to Yield from the template - but we should remove the loadingText if there's a block given.

i thiiiiink the syntax is like

{{#if blockGiven}}
  {{yield}}
{{else}}
  ... original template ...
{{/if}}

@davidgoli
Copy link
Collaborator Author

The correct property is hasBlock http://emberjs.com/api/classes/Ember.Component.html#property_hasBlock

... and done

@hhff
Copy link
Collaborator

hhff commented Aug 18, 2015

Thanks @davidgoli - can we get a lil' test for this?

@davidgoli
Copy link
Collaborator Author

@hhff sure - mind if I add the ember-cli-htmlbars-inline-precompile package to write an integration test?

@davidgoli
Copy link
Collaborator Author

@hhff Test added. Note that I had to revert to the old-style template helper that is supported by Ember 1.10 (which is the baseline for ember-infinity) but is deprecated in Ember 1.13. I also added the htmlbars-inline-precompile package per current best practice (https://github.com/rwjblue/ember-qunit#component-unit-tests)

@hhff
Copy link
Collaborator

hhff commented Aug 27, 2015

@davidgoli - if you merge from master this guy should be passing

@hhff
Copy link
Collaborator

hhff commented Aug 27, 2015

Also - can we please add a short description of how to use this to the README ?

@davidgoli
Copy link
Collaborator Author

@hhff Merged, fixed, and README updated.

@hhff
Copy link
Collaborator

hhff commented Aug 29, 2015

awesome - can you squash this?

@davidgoli
Copy link
Collaborator Author

Squash it? I don't think I have merge permission, if that's what you mean.

@hhff
Copy link
Collaborator

hhff commented Aug 30, 2015

Oh i just mean squash the commits @davidgoli

https://robots.thoughtbot.com/git-interactive-rebase-squash-amend-rewriting-history

We just do this so the Changelog is super clean and clear :)

@davidgoli
Copy link
Collaborator Author

@hhff how does this look?

@hhff
Copy link
Collaborator

hhff commented Aug 31, 2015

looks great! Is there a deprecation warning when using the template helper?

@davidgoli
Copy link
Collaborator Author

oh yeah I think so... in reality, it means the same thing as hasBlock, but always returns false on later versions. But, it's still necessary for backwards compatibility as well. What's your recommendation for how to handle this?

@davidgoli
Copy link
Collaborator Author

ie, create a helper, or what?

@hhff
Copy link
Collaborator

hhff commented Aug 31, 2015

I'd say the move is to use emberVersionIs in the component to wrap both of those.

We're already using it in the route.js here: https://github.com/hhff/ember-infinity/blob/master/addon/mixins/route.js#L2 so it should be easy to add 👍

@davidgoli
Copy link
Collaborator Author

Looks like that's not quite as easy for the Component case, since hasBlock is a helper not a component property. Fortunately, there's an officially-endorsed polyfill solution: emberjs/ember.js#11430 (comment)

@davidgoli
Copy link
Collaborator Author

Yeah that's passing ember-1.12, ember-1.13, ember-release, and ember-canary locally. Only question is where to put the polyfill.

import emberVersionIs from 'ember-version-is';

if (emberVersionIs('lessThan', '1.13.0')) {
Ember.Component.reopen({
Copy link
Collaborator

Choose a reason for hiding this comment

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

This is going to effect all components in the host application. Could we do it in the extend call below somehow instead? I don't want to effect the rest of a developer's application. Worse comes to worse - we could do the reopen before the export just on the infinity-loader

Thanks @davidgoli !

@davidgoli
Copy link
Collaborator Author

@hhff isolated the changes to only this component. Please take another look.

yield from blueprint, with blockGiven

do the same for the app/templates template

use hasBlock instead

use old-style template helper

use template helper in blueprint as well

update README with instructions

add additional check for hasBlock to pass 2.0+
@davidgoli
Copy link
Collaborator Author

whoops, forgot to add -u before pushing.

hhff added a commit that referenced this pull request Sep 2, 2015
@hhff hhff merged commit 687c9ac into adopted-ember-addons:master Sep 2, 2015
@hhff
Copy link
Collaborator

hhff commented Sep 2, 2015

THE MAN! thankyou @davidgoli !

@hhff
Copy link
Collaborator

hhff commented Sep 2, 2015

Published: + [email protected]

@kxcrl kxcrl mentioned this pull request Oct 7, 2015
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