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

Adds lazyRender support #75

Merged
merged 1 commit into from Oct 16, 2017
Merged

Conversation

@Duder-onomy
Copy link
Contributor

Duder-onomy commented Oct 10, 2017

This PR adds support to defer the render of the yield block in the ember-attacher-inner component.

The motivation would be to defer the initialization work of the attached items until the user interacts with it. In cases where the yielded contents are complex, this is preferred over having many items initialize which might or might not ever be used.

Adds a new property called lazyRender to the parent ember-attacher component.
This lazyRender property defaults to false to maintain current expected behavior.
Adds tests for lazyRendering to ensure contents are not rendered until after interacting.
Adds working example to the dummy app.

@kybishop, I am looking for some feedback. Are there any additional tests or cases you want explored?
I believe that once merged you will also need to deploy the dummy app to github pages.
Also, I was not sure if this should be in the readme or not as many of the options are not currently enumerated there.

Closes #74
Closes #64 (in my opinion)

@kybishop

This comment has been minimized.

Copy link
Owner

kybishop commented Oct 11, 2017

Thanks for the PR @Duder-onomy! I'll have some time to look at in depth tonight and tomorrow.

Copy link
Owner

kybishop left a comment

Awesome work @Duder-onomy! A few small changes requested, then ready to merge.

@@ -18,6 +18,8 @@ export default Component.extend({
init() {
this._super(...arguments);

this.set('_shouldRenderYieldBlock', !this.get('lazyRender'));

This comment has been minimized.

Copy link
@kybishop

kybishop Oct 14, 2017

Owner

_shouldRenderYieldBlock could be moved into a computed that depends on 'lazyRender'. Also, I think we should call it _shouldRender since it also takes care of rendering the arrow and circle (for the fill animation).

_shouldRender: computed('lazyRender', function() {
  return !this.get('lazyRender');
})

Setting this property on our first show event should still work as expected.

Side note: As a matter of best practices, use this.myProperty = something in init since this.set() is used to notify ember of property changes. Since we're initializing the component, there is no need to notify of changes. Basically comes down to a small performance win by avoiding unesc work.

@@ -52,6 +52,7 @@ export default Component.extend({
showDuration: DEFAULTS.showDuration,
showOn: DEFAULTS.showOn,
target: null,
lazyRender: DEFAULTS.lazyRender,

This comment has been minimized.

Copy link
@kybishop

kybishop Oct 14, 2017

Owner

I'd love to keep this list alphabetically sorted

@@ -16,5 +16,6 @@ export default {
showDelay: 0,
showDuration: 300,
showOn: 'mouseenter focus',
tooltipClass: 'ember-attacher-popper ember-attacher-tooltip'
tooltipClass: 'ember-attacher-popper ember-attacher-tooltip',
lazyRender: false

This comment has been minimized.

Copy link
@kybishop

kybishop Oct 14, 2017

Owner

I'd love to keep this list alphabetically sorted

@@ -7,7 +7,7 @@
placement=placement
popperContainer=popperContainer
renderInPlace=renderInPlace
target=target as |emberPopper|}}
target=target as |emberPopper|}}

This comment has been minimized.

Copy link
@kybishop

kybishop Oct 14, 2017

Owner

Looks like some extra whitespace snuck in here

@@ -13,5 +13,6 @@ export default Service.extend({
showDelay: 0,
showDuration: 300,
showOn: 'click',
target: '.target-plz'
target: '.target-plz',
lazyRender: false

This comment has been minimized.

Copy link
@kybishop

kybishop Oct 14, 2017

Owner

I'd love to keep this list alphabetically sorted

@@ -40,6 +40,7 @@
showDelay=popoverData.showDelay
showDuration=popoverData.showDuration
showOn=popoverData.showOn
lazyRender=popoverData.lazyRender

This comment has been minimized.

Copy link
@kybishop

kybishop Oct 14, 2017

Owner

alphabetically sorted please :)

@@ -304,6 +305,22 @@
{{/power-select}}"
</centered>
</hbox>
{{#unless isConfiguringTooltip}}

This comment has been minimized.

Copy link
@kybishop

kybishop Oct 14, 2017

Owner

Is there a particular reason to hide this when people are playing with attach-tooltip?

Also, let's keep this alphabetically sorted within the list of configurables :)

@Duder-onomy Duder-onomy force-pushed the Duder-onomy:add-lazy-render-support branch 3 times, most recently from 0094f61 to b0432f8 Oct 15, 2017
Greg Larrenaga
@Duder-onomy Duder-onomy force-pushed the Duder-onomy:add-lazy-render-support branch from b0432f8 to 6fff8cf Oct 15, 2017
@Duder-onomy

This comment has been minimized.

Copy link
Contributor Author

Duder-onomy commented Oct 15, 2017

Thanks for the feedback @kybishop. I think I addressed everything you mentioned and added a second test to ensure the default current behavior when lazyRender is unset.

Looking forward to getting this in. Been enjoying using ember-attacher recently, having ember-popper available on the yielded hash is a clutch move BTW. Saved me once already.

@kybishop kybishop merged commit 6c20031 into kybishop:master Oct 16, 2017
1 check passed
1 check passed
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@kybishop

This comment has been minimized.

Copy link
Owner

kybishop commented Oct 16, 2017

Merged! Thanks a ton @Duder-onomy. I'll have this out with the next release, which will be before EOW

@kybishop kybishop mentioned this pull request Oct 16, 2017
@Duder-onomy

This comment has been minimized.

Copy link
Contributor Author

Duder-onomy commented Oct 16, 2017

RAWK!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
2 participants
You can’t perform that action at this time.