FLUID-5534: UIO responsiveness and unit testing #844

ghost · 2017-09-06T14:20:30Z

Expanded unit tests, updated fluid.prefs.separatedPanel.scrollToPanel to constrain panelIndex within the range of panels, added arrows for navigation for responsive version.
https://issues.fluidproject.org/browse/FLUID-5534

Infusion-Docs PR: fluid-project/infusion-docs#127

Requires tests

The interactions for the arrows weren't working in IE, so we are hiding them instead. This is still just an interim solution, and hopefully the final solution will be supported across all supported browsers.

Also removed duplicate id's from separated panel markup

Added an example of using full page prefs editor but with separated panel styling and responsive interactions.

amb26 · 2017-09-06T15:29:29Z

src/framework/preferences/js/ArrowScrolling.js

+            }
+        },
+        invokers: {
+            scrollToPanel: {


This should be a modellised component rather than using DOM coordinates as an implicit model - also the range limiting should be done within the model space using a relay. See https://github.com/fluid-project/infusion/blob/master/src/components/textfieldControl/js/TextfieldSlider.js#L64 for examples of the latter.

Rather than a "scrollToPanel" invoker, the component should expose "panelIndex" and "panelCount" as model elements, with the final line "scrollLeft" implemented as a modelListener to the former.

@amb26 Thanks for the comment! I'm not sure I totally understand though. Do you mind going through it step-by-step?

@grrrero, a similar example that has implemented @amb26's suggestion is the scrolling feature in the language panel of the first discovery tool.

The panelIndex model value in @amb26's comment serves the same purpose as the selectedLang n the example. It keeps track of the current panel index. A click on arrow keys triggers a model change on the panelIndex value (to the scrollIndex value you calculated here). Then the model listener on panelIndex will perform the scrolling.

@amb26, correct me if this explanation doesn't seem right.

@grrrero and @cindyli I think this may be similar to https://github.com/GPII/first-discovery/blob/master/src/js/nav.js

@grrrero I've made some changes to finish up the work we started the other day to modelize panelIndex. I've submit a pull request against your branch ( https://github.com/grrrero/infusion/pull/1 ). You should review those changes and merge them into your branch if you are happy with them. You will need to add more tests to SeparatedPanelPrefsEditorTests for verifying the panelIndex after scrolling and to also trigger scrolling by using the change applier to change the panelIndex value.

cindyli · 2017-09-07T14:46:29Z

tests/framework-tests/preferences/js/SeparatedPanelPrefsEditorResponsiveTests.js

+
+    fluid.tests.clickArrow = function (elm, direction) {
+        var keyEvent = $.Event("click");
+        keyEvent.offsetX = direction ? elm.width() : 0;


It's a bit indirect and confusing to use {LEFT: false, RIGHT: true} to calculate offsetX. What do you think about something like keyEvent.offsetX = direction === "RIGHT" ? elm.width() : 0;?

@cindyli Hi Cindy, that makes sense, and less confusing.

cindyli · 2017-09-07T14:59:59Z

tests/framework-tests/preferences/js/SeparatedPanelPrefsEditorResponsiveTests.js

+        elm.trigger(keyEvent);
+    };
+
+    fluid.defaults("fluid.tests.separatedPanelResponsiveTester", {


Can you test the re-rendering at window resize too, when the window changes from a small screen to a wide screen and vice versa.

@cindyli Tested it, no bugs so far, and is there anything I should be looking out for?

@grrrero, I mean to add a unit test to make sure the window size change triggers the re-rendering of panels, not only via the manual test.

Started adding/updating unit tests but more should be added for verifying model values and scrolling via model changes in SeparatedPanelPrefsEditorTests

FLUID-5534: Modelizing panelIndex

amb26 · 2017-09-19T11:34:16Z

src/framework/preferences/js/ArrowScrolling.js

+        modelRelay: {
+            target: "panelIndex",
+            forward: {excludeSource: "init"},
+            singleTransform: {


Great improvement at modelising this component! Do put a namespace on this relay rule and the model listener which follows it in case anyone wants to override it someday

amb26 · 2017-09-19T11:39:04Z

src/framework/preferences/js/ArrowScrolling.js

+        return Math.max(0, panels.length - 1);
+    };
+
+    fluid.prefs.arrowScrolling.translateToScroll = function (that, event) {


Slightly better naming for this function/invoker - something like "eventToScrollIndex"?

amb26 · 2017-09-19T11:40:35Z

src/framework/preferences/js/ArrowScrolling.js

+            }
+        },
+        invokers: {
+            scrollToPanel: {


This invoker can be axed - the documentation that "scrollToIndex" is modelised is sufficient. Also it confusingly shares a name with the panelIndex listener

amb26 · 2017-09-19T11:43:17Z

src/framework/preferences/js/ArrowScrolling.js

+        },
+        model: {
+            // panelMaxIndex: null, // determined by the number of panels calculated after the onPrefsEditorMarkupReady event fired
+            // scrollToIndex: null, // the raw index set by translateToScroll, will be transformed to the panelIndex


I think we can get by with just one model field between scrollToIndex and panelIndex, and have translateToScroll attempt to write to panelIndex directly

@amb26 will the model transformation to limit the range of panelIndex still work in that case?

That's how it works in the original sample (the pager)

amb26 · 2017-09-19T11:44:11Z

src/framework/preferences/js/ArrowScrolling.js

+            panelIndex: 0
+        },
+        events: {
+            // beforeReset: null // should be provided by the fluid.prefs.prefsEditor grade


Best to uncomment this so there is a greater chance that the component can be self-sufficient (even if there are currently other reasons why it can't)

jobara · 2017-09-21T20:29:18Z

@amb26 the SeparatedPanelPrefsEditorResponsive-test.html test is not added to any of the all-tests.html files because the asynchrony needed to wait for the iframe to load is preventing the composite test from seeing any jqUnit tests. Do you know if there is a way to get around this?

amb26 · 2017-09-21T21:02:09Z

Yes - we have plenty of tests with asynchrony which work fine in the mix. The issue in this case will be most likely the test not beginning to execute sufficiently soon after the page load - this is a problem with QUnit's architecture which is well known. You have a 13ms window after the time QUnit itself is loaded to start queuing your tests, otherwise it will shut down assuming that there are none.

The problem is with the organisation of the test fixtures here - https://github.com/fluid-project/infusion/pull/844/files#diff-b55a73d1ab99b3bc42f1d4bc39a00744R251 - rather than waiting for the iframe to load and then starting to queue the tests afterwards, you should integrate the iframe loading into the test fixtures themselves, and start the tests immediately. This is similar to an issue that @michelled had with needing to wait for electron's app singleton to load before starting to run tests.

FLUID-5534: Added window resize test

FLUID-5534: Update tests to work in all-tests

amb26 · 2017-09-22T18:14:43Z

tests/framework-tests/preferences/js/SeparatedPanelPrefsEditorResponsiveTests.js

+            event = new Event(type);
+        } else {
+            event = document.createEvent("Event");
+            event.initEvent("resize", true, true);


Presumably this should use the supplied type here

FLUID-5534: Improvements to modelRelay

jobara · 2017-09-25T18:52:06Z

@amb26 I believe this is ready for more review. I've also added an Infusion-Docs PR.

amb26 · 2017-09-28T15:48:06Z

tests/framework-tests/preferences/js/SeparatedPanelPrefsEditorResponsiveTests.js

+        }]
+    });
+
+    fluid.tests.iframeSetup.load = function (that) {


Please put these definitions in a more descriptive namespace - since they are hardwired to start a pageEnhancer test. The name should at least indicate what broad area of the framework is being tests, e.g. fluid.tests.prefs or so

amb26 · 2017-09-28T15:49:34Z

tests/framework-tests/preferences/html/SeparatedPanelPrefsEditorResponsive-test.html

+
+    <!-- Test HTML -->
+    <!-- <div id="qunit-fixture"> -->
+        <iframe class="flc-iframeSetup-responsiveTests"></iframe>


Add a comment here and/or in the corresponding testDefs to the considerable research you've done in establishing why this is necessary on all current versions of IE (including linking to the jQuery ticket that holds a hopefully related selection bug)

amb26 · 2017-09-28T15:53:22Z

tests/framework-tests/preferences/js/SeparatedPanelPrefsEditorTests.js

@@ -131,8 +136,9 @@ https://github.com/fluid-project/infusion/raw/master/Infusion-LICENSE.txt
    fluid.tests.afterHideFunc1 = function () {


Retrospectively we should take the opportunity to fix namespacing of lots of definitions like this. Even browser tests should be written with the model that one day all these definitions might coexist as part of a single global namespace

FLUID-5534: Updated test namespace

FLUID-5534: Updating namespace for separated panel tests.

jobara · 2017-10-02T18:19:34Z

@amb26 this is ready for more review

amb26 · 2017-10-04T17:39:42Z

src/framework/preferences/js/ArrowScrolling.js

+                type: "fluid.transforms.limitRange",
+                input: "{that}.model.panelIndex",
+                min: 0,
+                max: "{that}.model.panelMaxIndex"


Now it is lovely!

Previously on install, stylus would not be compiled to CSS and on buildDist any generated CSS would be removed. However the CSS is now required for the responsive tests and must be in place.

FLUID-5534: Updating so that the tests will pass after an npm install.

jobara · 2017-10-05T14:49:06Z

@amb26 the build process has been updated so that buildStylus is run on prepublish

jobara and others added 15 commits May 1, 2017 14:33

FLUID-5534: Making arrows interactive for scrolling the panels

92065c8

Requires tests

FLUID-5534: Hiding the arrows in IE

7f39b7f

The interactions for the arrows weren't working in IE, so we are hiding them instead. This is still just an interim solution, and hopefully the final solution will be supported across all supported browsers.

FLUID-5534: Updating existing tests

cb01dd3

FLUID-5534: Starting tests for responsive UIO

c0c5576

Also removed duplicate id's from separated panel markup

FLUID-5534: Added a comment to the test

b53360e

Merge branch 'master' into FLUID-5534

810032f

Merge branch 'master' into FLUID-5534

4d92ec2

FLUID-5534: Added tests for slidingPanel.

192dd4c

FLUID-5534: Fixed and refactored existing responsive tests

d96f441

FLUID-5534: Added tests for panel positions.

7675572

FLUID-5534: Added test for scrolling panels with scrollToPanel invoker.

f7f42b0

FLUID-5534: Added tests for scrolling by clicking arrows.

1cb8b33

FLUID-5534: Updating style class name.

5b66f86

FLUID-5534: Refactored arrow functionality in separate grade.

ee3f98a

Added an example of using full page prefs editor but with separated panel styling and responsive interactions.

FLUID-5534: Unit tests for panelIndex bounds.

efe9df3

amb26 reviewed Sep 6, 2017

View reviewed changes

cindyli reviewed Sep 7, 2017

View reviewed changes

ghost changed the title ~~Fluid 5534~~ Fluid 5534: UIO responsiveness and unit testing Sep 11, 2017

ghost changed the title ~~Fluid 5534: UIO responsiveness and unit testing~~ FLUID-5534: UIO responsiveness and unit testing Sep 13, 2017

jobara and others added 3 commits September 15, 2017 09:50

FLUID-5534: Modelizing panelIndex

114b630

Started adding/updating unit tests but more should be added for verifying model values and scrolling via model changes in SeparatedPanelPrefsEditorTests

Merge pull request #1 from jobara/FLUID-5534

5c791ff

FLUID-5534: Modelizing panelIndex

FLUID-5534: making use of scrollToPanel invoker

78bed27

This was referenced Sep 18, 2017

FLOE-508: Updated Infusion for responsiveness. fluid-project/uio-wordpress-plugin#5

Merged

GPII-2424: Initial Implementation of the browser extension GPII/gpii-chrome-extension#2

Merged

amb26 reviewed Sep 19, 2017

View reviewed changes

FLUID-5534: Added a test for the window resize event.

4ba1dcf

jobara and others added 5 commits September 22, 2017 08:53

FLUID-5534: Adding iframe setup to tests. Now run in all-tests.

8ff61cc

FLUID-5534: Fixing event dispatch for IE11

1e8ee43

Merge pull request #4 from jobara/FLUID-5534

aef6a42

FLUID-5534: Added window resize test

FLUID-5534: Using sequence injection to setup tests.

273c97c

Merge pull request #5 from jobara/FLUID-5534

c6e6178

FLUID-5534: Update tests to work in all-tests

amb26 reviewed Sep 22, 2017

View reviewed changes

jobara and others added 3 commits September 25, 2017 09:11

FLUID-5534: Fixed bug in triggerDOMEvent function

d32ae11

FLUID-5534: Simplifying model by using panelIndex directly in transform

b222406

Merge pull request #6 from jobara/FLUID-5534

04adcb3

FLUID-5534: Improvements to modelRelay

This was referenced Sep 25, 2017

FLUID-6180: Adding persistence for adjuster selection in responsive display #853

Merged

FLUID-5534: Added API change information fluid-project/infusion-docs#127

Merged

amb26 reviewed Sep 28, 2017

View reviewed changes

jobara and others added 6 commits October 2, 2017 08:27

Merge branch 'master' into FLUID-5534

3955c31

FLUID-5534: Improving namespace for responsive tests.

ce0f79a

FLUID-5534: Adding comment about requiring two iframes in test.

caacd78

Merge pull request #7 from jobara/FLUID-5534

1c5a4d3

FLUID-5534: Updated test namespace

FLUID-5534: Updating namespace for separated panel tests.

98fc951

Merge pull request #8 from jobara/FLUID-5534

fe4b215

FLUID-5534: Updating namespace for separated panel tests.

amb26 reviewed Oct 4, 2017

View reviewed changes

jobara and others added 2 commits October 5, 2017 10:37

FLUID-5534: Updating so that the tests will pass after an npm install.

0f37cec

Previously on install, stylus would not be compiled to CSS and on buildDist any generated CSS would be removed. However the CSS is now required for the responsive tests and must be in place.

Merge pull request #9 from jobara/FLUID-5534

5857a79

FLUID-5534: Updating so that the tests will pass after an npm install.

amb26 merged commit 5857a79 into fluid-project:master Oct 5, 2017

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

FLUID-5534: UIO responsiveness and unit testing #844

FLUID-5534: UIO responsiveness and unit testing #844

ghost commented Sep 6, 2017 •

edited by jobara

Loading

amb26 Sep 6, 2017

ghost Sep 8, 2017

cindyli Sep 10, 2017

jobara Sep 13, 2017

jobara Sep 15, 2017

cindyli Sep 7, 2017

ghost Sep 8, 2017

cindyli Sep 7, 2017

ghost Sep 8, 2017

cindyli Sep 8, 2017

amb26 Sep 19, 2017

amb26 Sep 19, 2017

amb26 Sep 19, 2017

amb26 Sep 19, 2017

jobara Sep 19, 2017 •

edited

Loading

amb26 Sep 22, 2017

amb26 Sep 19, 2017

jobara commented Sep 21, 2017

amb26 commented Sep 21, 2017

amb26 Sep 22, 2017

jobara commented Sep 25, 2017 •

edited

Loading

amb26 Sep 28, 2017

amb26 Sep 28, 2017

amb26 Sep 28, 2017

jobara commented Oct 2, 2017

amb26 Oct 4, 2017

jobara commented Oct 5, 2017

		@@ -131,8 +136,9 @@ https://github.com/fluid-project/infusion/raw/master/Infusion-LICENSE.txt
		fluid.tests.afterHideFunc1 = function () {

FLUID-5534: UIO responsiveness and unit testing #844

FLUID-5534: UIO responsiveness and unit testing #844

Conversation

ghost commented Sep 6, 2017 • edited by jobara Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jobara Sep 19, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jobara commented Sep 21, 2017

amb26 commented Sep 21, 2017

Choose a reason for hiding this comment

jobara commented Sep 25, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jobara commented Oct 2, 2017

Choose a reason for hiding this comment

jobara commented Oct 5, 2017

ghost commented Sep 6, 2017 •

edited by jobara

Loading

jobara Sep 19, 2017 •

edited

Loading

jobara commented Sep 25, 2017 •

edited

Loading