Skip to content
This repository has been archived by the owner on Jul 30, 2018. It is now read-only.

Support css transitions options in app projector #69

Merged
merged 3 commits into from
Oct 5, 2016

Conversation

agubler
Copy link
Member

@agubler agubler commented Oct 3, 2016

No description provided.

@@ -45,6 +45,7 @@
"grunt-tslint": "^3.1.0",
"grunt-typings": ">=0.1.5",
"intern": "^3.2.3",
"sinon": "1.14.1",
Copy link
Contributor

Choose a reason for hiding this comment

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

Why pin to such an old version?

Copy link
Member Author

Choose a reason for hiding this comment

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

ah that is because I took it from core

Copy link
Member

Choose a reason for hiding this comment

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

word of caution, be careful, there be beasts in them sinon versions... if you find something that works, pin it...

Copy link
Member Author

Choose a reason for hiding this comment

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

😈

@@ -234,6 +234,10 @@ function resolveOptions(registry: ReadOnlyRegistry, registryProvider: RegistryPr
return options;
}

function getTransitionOptionFromProjector(element: Element): boolean {
return element.getAttribute('data-css-transitions') === 'true';
Copy link
Contributor

Choose a reason for hiding this comment

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

Would be cool if this worked with just <app-projector data-css-transitions>.

Copy link
Member Author

Choose a reason for hiding this comment

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

sure I'll add support for that

Copy link
Member Author

Choose a reason for hiding this comment

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

Something like element.hasAttribute('data-css-transitions') and ignoring the value?

Copy link
Member Author

Choose a reason for hiding this comment

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

should support, <app-projector data-css-transitions>, <app-projector data-css-transitions="true"> and <app-projector data-css-transitions="false"> now

Copy link
Member Author

Choose a reason for hiding this comment

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

any truthy value that is not === 'true' will be treated as false.

'<app-projector data-css-transition> attribute': {
'is false if empty'() {
return app.realize(root).then(() => {
assert.isTrue(projectorSpy.calledWith( match({ cssTransitions: false })));
Copy link
Contributor

Choose a reason for hiding this comment

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

In other projects I've stopped using Sinon's assertions. This should also work:

const { args: [ { cssTransitions } ] } = projectorSpy.firstCall;
assert.isTrue(cssTransitions);

Copy link
Member Author

Choose a reason for hiding this comment

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

any particular reason?

Copy link
Contributor

Choose a reason for hiding this comment

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

any particular reason?

Wait you can't read my mind? 😞

It's partly because I find it too hard to memorize Sinon's assertion APIs. But also because AVA uses power-assert and I tend to write boolean expressions as my test assertions.

Copy link
Member Author

Choose a reason for hiding this comment

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

I am not overly fussed, I'll update if there is overwhelming support to do so?

Copy link
Contributor

Choose a reason for hiding this comment

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

Nah it's fine as is 😄

@agubler
Copy link
Member Author

agubler commented Oct 3, 2016

plus I need to add sinon to the loader maps

@agubler agubler added this to the 2016.10 milestone Oct 3, 2016
@agubler agubler self-assigned this Oct 3, 2016
@agubler
Copy link
Member Author

agubler commented Oct 3, 2016

Requires a release of widgets

@agubler agubler merged commit 5afdd27 into dojo:master Oct 5, 2016
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants