EZP-27782: All tabs, from unrelated blocks, become unselected after clicking on any other tab #36

sunpietro · 2017-08-17T12:40:42Z

https://jira.ez.no/browse/EZP-27782

Now, the script is looking for the closest tabs container that contains selected tabs instead of searching in whole page document.

The HTML markup where an issue had been spotted:

<ez-server-side-content>
    <section class="ez-tabs ez-dashboard-tabs">
        <ul class="ez-tabs-list">
            <li class="ez-tabs-label is-tab-selected"><a href="#all_content">Content</a></li>
            <li class="ez-tabs-label"><a href="#all_media">Media</a></li>
        </ul>

        <ez-asynchronous-block class="ez-tabs-panel is-tab-selected" id="all_content" url="/all_content"></ez-asynchronous-block>
        <ez-asynchronous-block class="ez-tabs-panel" id="all_media" url="/all_media"></ez-asynchronous-block>
    </section>

    <section class="ez-tabs ez-dashboard-tabs">
        <ul class="ez-tabs-list">
            <li class="ez-tabs-label is-tab-selected"><a href="#my_content">My content</a></li>
            <li class="ez-tabs-label"><a href="#my_drafts">My drafts</a></li>
            <li class="ez-tabs-label"><a href="#my_media">My media</a></li>
        </ul>

        <ez-asynchronous-block class="ez-tabs-panel is-tab-selected" id="my_content" url="/my_content"></ez-asynchronous-block>
        <ez-asynchronous-block class="ez-tabs-panel" id="my_drafts" url="/my_drafts"></ez-asynchronous-block>
        <ez-asynchronous-block class="ez-tabs-panel" id="my_media" url="/my_media"></ez-asynchronous-block>
    </section>
</ez-server-side-content>

andrerom · 2017-08-17T13:02:24Z

mixins/js/ez-tabs.js

-                forEach.call(this.querySelectorAll('.' + TAB_IS_SELECTED), function (element) {
-                    element.classList.remove(TAB_IS_SELECTED);
-                });
+                [].forEach.call(selectedTabs, (element) => element.classList.remove(TAB_IS_SELECTED));


however I don't find the line here particular easy to read, one thing is the short form for functions which I'm not used to, but even Array.prototype would have been cleaner then the empty array. But thats somewhat a style preference, and if this is more the style things are done now I can perhaps get used to it.

I agree with @andrerom on [].forEach vs. Array.prototype.forEach, especially given this change is completely unrelated to the issue (and useless) so to keep the patch as short as possible, I would only change the first parameter of forEach.call.

that said thanks to the web components polyfill, you can even use forEach method directly on NodeList returned by querySelectorAll.
(see #30 (comment))

For fat arrow vs. regular function, we tend to use fat arrow to avoid using bind but that's fine this way as well.

dpobel

instead of searching in whole page document.

normally we don't search in the whole page but only within the custom element on which ez-tabs mixin is applied.

So I guess to reproduce the issue you must have 2 different series of tabs in a custom element having this mixin, right ? Could you please add in the PR description a minimal markup example to reproduce the issue ?

dpobel · 2017-08-21T13:16:31Z

mixins/js/ez-tabs.js

@@ -1,5 +1,5 @@
 window.eZ = window.eZ || {};
-
+/* jshint esversion:6 */


~~that's not needed normally, see https://github.com/ezsystems/hybrid-platform-ui-core-components/blob/master/.eslintrc.json#L8~~

I've just notice the jshint part, we don't use jshint here, so please remove that line.

dpobel · 2017-08-21T13:17:11Z

mixins/js/ez-tabs.js

@@ -107,23 +107,25 @@ window.eZ = window.eZ || {};
             *
             * @param {HTMLElement} panel
             * @param {HTMLElement} tabsLabel
+             * @param event {Event} click event


for consistency please put the type first

dpobel · 2017-08-21T13:30:54Z

mixins/js/ez-tabs.js

-                forEach.call(this.querySelectorAll('.' + TAB_IS_SELECTED), function (element) {
-                    element.classList.remove(TAB_IS_SELECTED);
-                });
+                [].forEach.call(selectedTabs, (element) => element.classList.remove(TAB_IS_SELECTED));


I agree with @andrerom on [].forEach vs. Array.prototype.forEach, especially given this change is completely unrelated to the issue (and useless) so to keep the patch as short as possible, I would only change the first parameter of forEach.call.

that said thanks to the web components polyfill, you can even use forEach method directly on NodeList returned by querySelectorAll.
(see #30 (comment))

For fat arrow vs. regular function, we tend to use fat arrow to avoid using bind but that's fine this way as well.

sunpietro · 2017-08-21T13:58:46Z

@dpobel updated description and corrected minor issues pointed.

dpobel

besides the inline comment, you also need to write a unit test where you reproduce the issue when there are several .ez-tabs elements.

dpobel · 2017-08-22T12:52:43Z

mixins/js/ez-tabs.js

@@ -78,7 +78,7 @@ window.eZ = window.eZ || {};

                    e.preventDefault();
                    if ( this._dispatchTabChange(label, panel) ) {
-                        this._changeTab(panel, label);
+                        this._changeTab(panel, label, e);


instead of passing the event object until _unselectTab, I think you should compute the tab container here to have:

const tabContainer = e.target.closest('.ez-tabs'); this._changeTab(tabContainer, panel, label);

…licking on any other tab

sunpietro · 2017-08-25T09:59:35Z

@dpobel I've added updated tests. Any comments are welcome.

dpobel

weird TravisCI did not run on this PR, I've changed a settings let's see if it sees the PR when you'll push changes (also rebase on master so that you get a small fix to npm run lint 5b545c3).
But in any case please run the tests locally (https://github.com/ezsystems/hybrid-platform-ui-core-components/blob/master/README.md#developers-tasks)

dpobel · 2017-08-30T09:40:40Z

mixins/js/ez-tabs.js

@@ -107,21 +109,24 @@ window.eZ = window.eZ || {};
             *
             * @param {HTMLElement} panel
             * @param {HTMLElement} tabsLabel
+             * @param {HTMLElement} tabs container DOM node


the parameter name is tabsContainer not tabs

dpobel · 2017-08-30T09:40:59Z

mixins/js/ez-tabs.js


                tabsLabel.classList.add(TAB_IS_SELECTED);
                panel.classList.add(TAB_IS_SELECTED);
            }

            /**
             * Unselects currently selected tabs
+             *
+             * @param {HTMLElement} tabs container DOM node


same on doc parameter name

dpobel · 2017-08-30T09:48:39Z

test/mixins/ez-tabs.html

+            </template>
+        </test-fixture>
+
+        <test-fixture id="AdvancedTestFixture">


it's not really an advanced test but a regression test for the case where there are several tabs series, so I would call it SeveralTabsSeriesTestFixture

dpobel · 2017-08-30T09:51:44Z

test/mixins/ez-tabs.html

+                        </div>
+                    </div>
+                </ez-test-tabs>
+                <ez-test-tabs>


with this markup, you don't test the bug you are fixing which is happening is you have several tabs series in the custom element where Tabs mixin is applied. So you should have only one ez-test-tabs element with at least 2 tabs markup.

dpobel · 2017-08-30T09:54:04Z

test/mixins/js/ez-tabs.js

-                bubbles: true,
-            }));
-        }
+        element = fixture('BasicTestFixture');


this needs to be inside beforeEach so that each test starts with a fresh element/markup (see https://www.polymer-project.org/2.0/docs/tools/tests#test-fixtures)

dpobel · 2017-08-30T09:54:34Z

test/mixins/js/ez-tabs.js

+        const CLASS_TAB_SELECTED = 'is-tab-selected';
+        const SELECTOR_TAB_SELECTED = '.is-tab-selected';
+
+        element = fixture('AdvancedTestFixture');


should be in beforeEach as well

dpobel · 2017-08-30T09:55:21Z

test/mixins/js/ez-tabs.js

@@ -105,4 +103,38 @@ describe('ez-tabs', function () {
            });
        });
    });
+
+    describe('switch tab in a correct container', function () {


describe('several tabs series', function () {

dpobel · 2017-08-30T10:15:53Z

test/mixins/js/ez-tabs.js

+            panel = element.querySelector(link.getAttribute('href'));
+        });
+
+        it('should change tab in a correct tabs container', function () {


actually here we want to test 2 things so I think you should write 2 tests:

should change tab which should be the same as 'switch tab' / 'should change tab' (so you should extract the code of it in a function for instance called testChangeTab and call it in both cases)

should leave others tabs intact which will also call testChangeTab but in addition check that the other tabs have not changed (for that the easiest is probably to compare .innerHTML of the other tab container before and after the tab change)

dpobel · 2017-08-30T10:18:13Z

test/ez-content-view.html

-                        <div class="ez-tabs-panel" id="tab2">
-                            Some other content
-                        </div>
-                        <ez-asynchronous-block id="async-tab"></ez-asynchronous-block>


since mixins/js/ez-tabs.js is also included in this file, you need to add the SeveralTabsSeriesTestFixture here as well (but make sure to use ez-content-view not ez-test-tabs)

dpobel · 2017-08-30T10:19:02Z

test/ez-server-side-content.html

-                        <div class="ez-tabs-panel" id="tab2">
-                            Some other content
-                        </div>
-                        <ez-asynchronous-block id="async-tab"></ez-asynchronous-block>


and same here, mixins/js/ez-tabs.js is included in this file as well so a SeveralTabsSeriesTestFixture is needed

andrerom approved these changes Aug 17, 2017

View reviewed changes

andrerom reviewed Aug 17, 2017

View reviewed changes

dpobel suggested changes Aug 21, 2017

View reviewed changes

dpobel suggested changes Aug 22, 2017

View reviewed changes

Piotr Nalepa added 2 commits August 25, 2017 11:57

EZP-27782: All tabs, from unrelated blocks, become unselected after c…

3795708

…licking on any other tab

Updated tests

31320cc

dpobel suggested changes Aug 30, 2017

View reviewed changes

sunpietro closed this Sep 26, 2017

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

EZP-27782: All tabs, from unrelated blocks, become unselected after clicking on any other tab #36

EZP-27782: All tabs, from unrelated blocks, become unselected after clicking on any other tab #36

sunpietro commented Aug 17, 2017 •

edited

andrerom Aug 17, 2017 •

edited

dpobel Aug 21, 2017

dpobel left a comment

dpobel Aug 21, 2017 •

edited

dpobel Aug 21, 2017

dpobel Aug 21, 2017

sunpietro commented Aug 21, 2017

dpobel left a comment

dpobel Aug 22, 2017

sunpietro commented Aug 25, 2017

dpobel left a comment

dpobel Aug 30, 2017 •

edited

dpobel Aug 30, 2017

dpobel Aug 30, 2017

dpobel Aug 30, 2017

dpobel Aug 30, 2017

dpobel Aug 30, 2017

dpobel Aug 30, 2017

dpobel Aug 30, 2017

dpobel Aug 30, 2017

dpobel Aug 30, 2017

		@@ -1,5 +1,5 @@
		window.eZ = window.eZ \|\| {};

		/* jshint esversion:6 */

EZP-27782: All tabs, from unrelated blocks, become unselected after clicking on any other tab #36

EZP-27782: All tabs, from unrelated blocks, become unselected after clicking on any other tab #36

Conversation

sunpietro commented Aug 17, 2017 • edited

andrerom Aug 17, 2017 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dpobel left a comment

Choose a reason for hiding this comment

dpobel Aug 21, 2017 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sunpietro commented Aug 21, 2017

dpobel left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sunpietro commented Aug 25, 2017

dpobel left a comment

Choose a reason for hiding this comment

dpobel Aug 30, 2017 • edited

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

sunpietro commented Aug 17, 2017 •

edited

andrerom Aug 17, 2017 •

edited

dpobel Aug 21, 2017 •

edited

dpobel Aug 30, 2017 •

edited