GPII-2933 Common Event Bindings #13

sgithens · 2019-09-14T00:07:48Z

@amb26 @the-t-in-rtf I'm going to look at adding a few unit tests, but if you wanted to take an initial look at this and give me any feedback, that'd be helpful

gpii-bot · 2019-09-14T00:22:16Z

CI job failed: https://ci.gpii.net/job/gpii-binder-tests/2/

the-t-in-rtf · 2019-09-16T09:14:36Z

Great idea, and very useful, as we do this kind of grunt work all the time in downstream grades. Although I could look at stuff in isolation, it'd be helpful to see tests so that I can see examples of what you're passing and what the intended effect is. I'm happy with these being synthetic testem-friendly tests, simulating keyboard input crudely rather than using gpii-webdriver.

Also, linting! Dude! (I was wondering how the heck you managed to break the tests without adding tests for your work. That's how.)

sgithens · 2019-11-20T16:25:53Z

Hi @the-t-in-rtf . I added a few unit tests for this, and everything should lint now. :)

gpii-bot · 2019-11-20T17:10:49Z

CI job failed: https://ci.gpii.net/job/gpii-binder-tests/4/

tests/static/js/jqunit/jqUnit-binder-commonEventBindings.js

src/js/common-event-bindings.js

tests/static/js/jqunit/jqUnit-binder-commonEventBindings.js

src/js/common-event-bindings.js

the-t-in-rtf · 2019-11-22T14:41:58Z

Hi, @sgithens, I'm trying to make sense of the associated ticket, you kind of describe in the title the change you're making, but I couldn't get more info as most of the links to example there are broken. I think I get it, but for posterity we need a clear summary.

Also, the new functionality needs to be described somewhere, perhaps in a new section towards the end of the README.

gpii-bot · 2019-11-23T11:10:20Z

CI job failed: https://ci.gpii.net/job/gpii-binder-tests/5/

sgithens · 2019-11-25T22:39:37Z

@the-t-in-rtf Updated the README and ticket. I believe this should be ready for another review now.

gpii-bot · 2019-11-25T23:25:34Z

CI job failed: https://ci.gpii.net/job/gpii-binder-tests/6/

README.md

src/js/common-event-bindings.js

the-t-in-rtf · 2019-11-26T10:19:54Z

src/js/common-event-bindings.js

+     * Function to process the markup binding decorators and create the events described
+     * by them. The markup needs to be rendered and settled before this can be called.
+     *
+     * @param {Object} that - The component itself.


I am the guiltiest party here, I do this all the time, but in later work, @amb26 and I have been adding the grade to these for purposes of clarity.

Make these more specific using the component grade for the type.

the-t-in-rtf · 2019-11-26T10:21:46Z

I left a few comments, but am happy with this. @amb26, can you look through this as well? I know you and @sgithens have already discussed this, it'd be good to confirm this roughly matches what you had in mind.

src/js/common-event-bindings.js

tests/static/js/jqunit/jqUnit-binder-commonEventBindings.js

amb26 · 2019-11-26T10:27:58Z

tests/static/js/jqunit/jqUnit-binder-commonEventBindings.js

+            paragraphClick: ".paragraph-click",
+            multipleEventInputButton: "#input-multiple-events-button"
+        },
+        markupEventBindings: {


Also supply a test which shows that these handlers can be overridden cleanly

Added a test where 1) Some are left as is, 2) some are overridden, and 3) some new one is introduced.

amb26 · 2019-11-26T10:28:47Z

tests/static/js/jqunit/jqUnit-binder-commonEventBindings.js

+        markupEventBindings: {
+            inputButton: {
+                method: "click",
+                args: ["{that}.handleInputClick"]


Supply further tests to validate the primitive kinds of "compact" IoC syntax we support which allows for args

Thanks for this tip, I didn't know this was possibly actually, and will simplify my usage in the capture tool, PPT, and elsewhere

gpii-bot · 2020-04-02T00:40:49Z

CI job failed: https://ci.gpii.net/job/gpii-binder-tests/11/

- Adding ability to pass an array of handlers to eventMarkupBindings - Adding unit tests for overriding bindings in sub grades - Adding unit tests for compact IoC invocation - Grammar and jsdoc fixes

sgithens · 2020-04-02T17:36:16Z

@amb26 @the-t-in-rtf I believe this should be ready for another round of review now

gpii-bot · 2020-04-02T17:42:25Z

CI job failed: https://ci.gpii.net/job/gpii-binder-tests/12/

the-t-in-rtf · 2020-04-03T12:46:44Z

The build failed because the VM couldn't be created:

+ vagrant up --provider virtualbox
The provider 'virtualbox' that was requested to back the machine
'windows10' is reporting that it isn't usable on this system. The
reason is shown below:

Vagrant has detected that you have a version of VirtualBox installed
that is not supported by this version of Vagrant. Please install one of
the supported versions listed below to use Vagrant:

4.0, 4.1, 4.2, 4.3, 5.0, 5.1, 5.2

OK to test again.

gpii-bot · 2020-04-03T13:35:29Z

CI job failed: https://ci.gpii.net/job/gpii-binder-tests/13/

the-t-in-rtf · 2020-04-06T07:21:35Z

Vagrant timeout this go around. Ok to test again, if it fails a third time we should rope in Alfredo to assist.

gpii-bot · 2020-04-06T08:10:54Z

CI job failed: https://ci.gpii.net/job/gpii-binder-tests/14/

the-t-in-rtf · 2020-04-07T10:09:37Z

==> windows10: ok 13 Firefox 73.0 - [734 ms] - Binder Tests: /tests/static/tests-binder-commonEventBindingsOverride.html
Build was aborted
Aborted by unknown
/opt/vagrant/embedded/gems/2.2.7/gems/winrm-2.3.4/lib/winrm/http/response_handler.rb:59:in `raise_if_auth_error': WinRM::WinRMAuthorizationError (WinRM::WinRMAuthorizationError)
	from /opt/vagrant/embedded/gems/2.2.7/gems/winrm-2.3.4/lib/winrm/http/response_handler.rb:51:in `raise_if_error'

Looks like the build was killed after timing out, I suspect issues with testem. I am testing locally at the moment and will advise shortly on how to proceed.

the-t-in-rtf · 2020-04-07T10:28:53Z

OK, there are two problems:

You need to merge with upstream master to pick up the fix for the Firefox hang issues described in GPII-4145.
Since upstream master now uses a potentia-ii version of Infusion, you'll need to update your dependencies in the new test fixtures to use the same infusion-all rollup that the rest of the tests use. Otherwise you're missing the now-required promises API.

With those two fixes your tests pass in Vagrant locally.

sgithens · 2020-04-07T18:30:41Z

@the-t-in-rtf Thanks for the sluething, try these updates now.

gpii-bot · 2020-04-07T18:43:29Z

CI job passed: https://ci.gpii.net/job/gpii-binder-tests/15/

sgithens · 2020-04-07T18:44:07Z

@the-t-in-rtf Yes! It passed

gpii-bot · 2020-04-10T20:21:03Z

CI job failed: https://ci.gpii.net/job/gpii-binder-tests/16/

sgithens · 2020-04-10T21:42:29Z

Looks like a timeout...

ok to test

gpii-bot · 2020-04-10T22:00:04Z

CI job passed: https://ci.gpii.net/job/gpii-binder-tests/17/

sgithens · 2020-04-10T22:01:30Z

@the-t-in-rtf Ok, this is passed, and I think ready for another review now.

the-t-in-rtf · 2020-05-04T13:43:07Z

src/js/common-event-bindings.js

+     */
+    fluid.defaults("gpii.binder.bindMarkupEvents", {
+        mergePolicy: {
+            decorators: "noexpand"


Why is this "decorators"?

In other words, is there a pattern we're emulating? Just curious.

the-t-in-rtf · 2020-05-04T13:46:29Z

src/js/common-event-bindings.js

+            onMarkupRendered: null
+        },
+        listeners: {
+            onMarkupRendered: "{that}.events.onDomBind.fire({that}, {that}.container)",


These should all be namespaced. I'd also suggest using the "long form" to this shorter notation. That way someone can at least opt out of part of the contract in a derived grade by changing the "funcName" to fluid.identity or the like.

the-t-in-rtf · 2020-05-04T13:51:32Z

src/js/common-event-bindings.js

+            if (node.length > 0) {
+                var name = "Decorator for DOM node with selector " + key + " for component " + fluid.dumpThat(that);
+                // val can be an array to support multiple event handlers
+                var handlerDecs = fluid.isArrayable(val) ? val : [val];


Why not just use fluid.makeArray here?

the-t-in-rtf · 2020-05-04T13:54:32Z

tests/static/js/jqunit/jqUnit-binder-commonEventBindings.js

+            }
+        },
+        listeners: {
+            "onCreate.markupEventBindings": "{that}.events.onMarkupRendered.fire()"


This is the kind of thing I'd also write out, what if someone needs to do other work before firing the event? I guess they can just rewrite the whole string.

the-t-in-rtf · 2020-05-04T13:57:40Z

tests/static/js/jqunit/jqUnit-binder-commonEventBindings.js

+        }
+    });
+
+    gpii.tests.binder.commonEventBindings.environment();


I guess it matters less on the browser side (I think?), but for consistency with the rest of the package, this should use fluid.test.runTests instead of calling the environment directly.

the-t-in-rtf · 2020-05-04T13:58:17Z

tests/static/js/jqunit/jqUnit-binder-commonEventBindingsOverride.js

+        }
+    });
+
+    gpii.tests.binder.commonEventBindingsOverride.environment();


We should either consistently use fluid.test.runTests or consistently not use it.

+1 since without runTests there is no teardown

the-t-in-rtf · 2020-05-04T13:59:18Z

tests/static/tests-binder-commonEventBindingsOverride.html

+        <script type="text/javascript" src="../../src/js/common-event-bindings.js"></script>
+    </head>
+    <body>
+        <h1 id="qunit-header">"Common Event Bindings" Component Tests</h1>


Please update the page and header title to distinguish this from the other tests.

the-t-in-rtf · 2020-05-04T14:00:21Z

tests/static/tests-binder-commonEventBindingsOverride.html

+        <!-- Test HTML, we have to hide this ourselves.  QUnit's styles put it offscreen, which doesn't work for us. -->
+        <div class="viewport-common-event-bindings" style="display:none">
+            <form>
+                <h3>Common Event Bindings...</h3>


I know it's mostly harmless, but for any human trying to troubleshoot the test, this should also be different than the other comment event bindings tests.

CLAassistant · 2021-08-23T15:51:09Z

Thank you for your submission! We really appreciate it. Like many open source projects, we ask that you sign our Contributor License Agreement before we can accept your contribution.
_{You have signed the CLA already but the status is still pending? Let us recheck it.}

GPII-2933 Common Event Bindings

669e19d

sgithens added 2 commits November 18, 2019 15:25

Merge remote-tracking branch 'GPII/master' into GPII-2933

e44d0f2

GPII-2933 Adding unit tests for common event bindings

c6f021b

the-t-in-rtf reviewed Nov 22, 2019

View reviewed changes

tests/static/js/jqunit/jqUnit-binder-commonEventBindings.js Outdated Show resolved Hide resolved

the-t-in-rtf reviewed Nov 22, 2019

View reviewed changes

src/js/common-event-bindings.js Show resolved Hide resolved

the-t-in-rtf reviewed Nov 22, 2019

View reviewed changes

tests/static/js/jqunit/jqUnit-binder-commonEventBindings.js Show resolved Hide resolved

the-t-in-rtf reviewed Nov 22, 2019

View reviewed changes

src/js/common-event-bindings.js Show resolved Hide resolved

sgithens added 2 commits November 23, 2019 01:31

GPII-2933 Fixing typo's and starting tests for keydown events.

b5a1070

GPII-2933 Adding support for multiple events in a single declaration

6e7d524

GPII-2933 JSDocs, README doco, minor grammar fixes.

b5c2d4d

the-t-in-rtf reviewed Nov 26, 2019

View reviewed changes

README.md Show resolved Hide resolved

the-t-in-rtf reviewed Nov 26, 2019

View reviewed changes

src/js/common-event-bindings.js Outdated Show resolved Hide resolved

the-t-in-rtf reviewed Nov 26, 2019

View reviewed changes

src/js/common-event-bindings.js Outdated Show resolved Hide resolved

the-t-in-rtf reviewed Nov 26, 2019

View reviewed changes

src/js/common-event-bindings.js Outdated Show resolved Hide resolved

the-t-in-rtf reviewed Nov 26, 2019

View reviewed changes

amb26 reviewed Nov 26, 2019

View reviewed changes

src/js/common-event-bindings.js Outdated Show resolved Hide resolved

amb26 reviewed Nov 26, 2019

View reviewed changes

tests/static/js/jqunit/jqUnit-binder-commonEventBindings.js Show resolved Hide resolved

amb26 reviewed Nov 26, 2019

View reviewed changes

GPII-2933 Updating jsdocs

2dd12a6

GPII-2933 Cleanup from PR

3bd28a7

- Adding ability to pass an array of handlers to eventMarkupBindings - Adding unit tests for overriding bindings in sub grades - Adding unit tests for compact IoC invocation - Grammar and jsdoc fixes

sgithens added 2 commits April 7, 2020 10:54

Merge remote-tracking branch 'GPII/master' into GPII-2933

0be5062

GPII-2933 Updated to infusion dist to match other tests.

d7ed31a

GPII-2933 Updating that types in jsdocs

cb18acc

the-t-in-rtf reviewed May 4, 2020

View reviewed changes

jobara changed the base branch from master to main October 22, 2020 13:58

GPII-2933 Common Event Bindings #13

Are you sure you want to change the base?

GPII-2933 Common Event Bindings #13

Uh oh!

Conversation

sgithens commented Sep 14, 2019

Uh oh!

gpii-bot commented Sep 14, 2019

Uh oh!

the-t-in-rtf commented Sep 16, 2019

Uh oh!

sgithens commented Nov 20, 2019

Uh oh!

gpii-bot commented Nov 20, 2019

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

the-t-in-rtf commented Nov 22, 2019

Uh oh!

gpii-bot commented Nov 23, 2019

Uh oh!

sgithens commented Nov 25, 2019

Uh oh!

gpii-bot commented Nov 25, 2019

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

the-t-in-rtf commented Nov 26, 2019

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

gpii-bot commented Apr 2, 2020

Uh oh!

sgithens commented Apr 2, 2020

Uh oh!

gpii-bot commented Apr 2, 2020

Uh oh!

the-t-in-rtf commented Apr 3, 2020

Uh oh!

gpii-bot commented Apr 3, 2020

Uh oh!

the-t-in-rtf commented Apr 6, 2020

Uh oh!

gpii-bot commented Apr 6, 2020

Uh oh!

the-t-in-rtf commented Apr 7, 2020

Uh oh!

the-t-in-rtf commented Apr 7, 2020

Uh oh!

sgithens commented Apr 7, 2020

Uh oh!

gpii-bot commented Apr 7, 2020

Uh oh!

sgithens commented Apr 7, 2020

Uh oh!

gpii-bot commented Apr 10, 2020

Uh oh!

sgithens commented Apr 10, 2020

Uh oh!

gpii-bot commented Apr 10, 2020

Uh oh!

sgithens commented Apr 10, 2020

Uh oh!

Choose a reason for hiding this comment