Fixed #70 - user now stays in the same tab while navigating around th… #289

m-suchorski · 2018-07-18T09:59:19Z

User now stays in the same tab while navigating around tests tree.

Description

Moving around the tree by using [ ] or arrows now automatically opens up url view instead of test view. User also stays on the same tab when moving between urls. If the same type of tab isn't found on the next/previous test url then first tab is selected.

Motivation and Context

Fix for issue: #70 - Navigating between pages should remain user on the same tab

Types of changes

Bug fix (non-breaking change which fixes an issue)
New feature (non-breaking change which adds functionality)
Breaking change (fix or feature that would cause existing functionality to not work as expected)

Checklist:

My code follows the code style of this project.
My change requires a change to the documentation.
I have updated the documentation accordingly.

I hereby agree to the terms of the AET Contributor License Agreement.

…und the tree

adamkosmala · 2018-07-18T10:07:28Z

report/src/main/webapp/app/components/keyboardShortcuts.directive.js

@@ -141,36 +142,41 @@ define(['angularAMD', 'userSettingsService'], function (angularAMD) {
            if (!_.isEmpty($(suiteContainer).nextAll(
                    '.aside-report:not(.is-hidden)').first().find(
                    '.test-name'))) {
+              currentTab = $('.nav-tabs > .nav-item').filter('.active').text().replace(/\s/g, '');


is it possible to have more than one .nav-item element with class active? if not, why not going without filtering?

Not at once, no. What would you recommend? I've tried adding class to the selector but it doesn't work as intended.

after discussion I'm ok with keeping it as it is

oliwerin · 2018-07-18T10:34:19Z

report/src/main/webapp/app/components/keyboardShortcuts.directive.js

          } else {
+            currentTab = $('.nav-tabs > .nav-item').filter('.active').text().replace(/\s/g, '');


Please add a $ prefix to the variable name to indicate it holds a jQuery object

Even though it's just plain text at the end ('screenlayout' for example)?

Ok, didn't notice - no need to worry about it then.

The name is very misleading though - rename it to currentTabLabel or something else more accurate

oliwerin · 2018-07-18T10:36:01Z

report/src/main/webapp/app/components/keyboardShortcuts.directive.js

+            if (!_.isEmpty($(suiteContainer).prevAll(
+                  '.aside-report:not(.is-hidden)').first()
+                  .find('.test-name'))) {
+              currentTab = $('.nav-tabs > .nav-item').filter('.active').text().replace(/\s/g, '');


Here you have active and in the lines below is-active - can we make it consistent?

.active is a class for tabs while .is-active is a class for sidebar menu, I didn't modify any html/css structure. I can ask @Skejven what he thinks about it and then maybe change it.

@oliwerin what is the problem with existing those two classes?
If you recommend to align them, please do so in a separate pull request.

@Skejven It's not a huge issue but it's a class representing certain state and ideally there should be only one - for better consistency, readability and maintainability.

oliwerin · 2018-07-18T11:09:09Z

report/src/main/webapp/app/components/keyboardShortcuts.directive.js

+      function clickOnTab() {
+        var currentTab = userSettingsService.getLastTab();
+        var nextTestTabs = $('.nav-tabs').children();
+        for(var i=0;i <nextTestTabs.length; i++) {


Why you're not using jQuery each for keeping it simpler and more consistent?

I will change that.

Could you also fix formatting? Thanks.

oliwerin · 2018-07-18T11:10:44Z

report/src/main/webapp/app/components/keyboardShortcuts.directive.js

+
+      function clickOnTab() {
+        var currentTab = userSettingsService.getLastTab();
+        var nextTestTabs = $('.nav-tabs').children();


Both lines - 248-249 - consider adding a $ prefix if they store jQuery collections

Here again currentTab is just plain text, will change that to currentTabLabel.
As to next line I will change that.

oliwerin · 2018-07-18T11:11:25Z

report/src/main/webapp/app/components/keyboardShortcuts.directive.js

+        }
+      }
+
+      var mutationObserver = new MutationObserver(callback);


Can you shortly explain why it's needed with config set to true for each property?

I've used this: https://developer.mozilla.org/en-US/docs/Web/API/MutationObserverInit
I've tried few configs and this one has been working properly for me.

Ok, but why do you need MutationObserver, whatever its config is?

It looks for changes in the DOM before it is displayed to the user.

Here, if I didn't use MutationObserver my screen would flick every time I changed tree URL, because selecting the tab is done by using .click();

With MutationObserver I can atually click something before it is displayed (but it already exists in the DOM), both the delay and the flickering is gone.

I've tried using timeout - but it is based on how many tests are performed, the more tests, the longer the timeout should be (and it varies from 10 to 50ms +). Then I've tried with window.onhashchange but it didn't change anything. I just needed something that would detect a change before user sees anything.

It's worth to keep in mind that MutationObserver with such an extensive config might be performance heavy. It also sounds to me that it's used to fix a problem that should not occur - I don't think that screen should flick every time you change the URL. Maybe it would be better to spend more time on it and fix it properly.
I would suggest at least adding a short comment explaining why MutationObserver is used so future developers working on it have a clear understanding.

malaskowski · 2018-07-18T13:05:20Z

report/src/main/webapp/app/components/keyboardShortcuts.directive.js

+            if (!_.isEmpty($(suiteContainer).prevAll(
+                  '.aside-report:not(.is-hidden)').first()
+                  .find('.test-name'))) {
+              currentTab = $('.nav-tabs > .nav-item').filter('.active').text().replace(/\s/g, '');


@oliwerin what is the problem with existing those two classes?
If you recommend to align them, please do so in a separate pull request.

malaskowski · 2018-07-18T13:06:54Z

report/src/main/webapp/app/components/keyboardShortcuts.directive.js

-            $previousElement.click();
-            currentItem.parents('.url-name').removeClass('is-active');
-            suiteContainer.addClass('is-expanded');
+            if (!_.isEmpty($(suiteContainer).prevAll(


Maybe it would be worth to put following 20 lines into a separate method? traverseTreeUp is a little to long now.

malaskowski · 2018-07-18T13:07:25Z

report/src/main/webapp/app/components/keyboardShortcuts.directive.js

+      function clickOnTab() {
+        var currentTab = userSettingsService.getLastTab();
+        var nextTestTabs = $('.nav-tabs').children();
+        for(var i=0;i <nextTestTabs.length; i++) {


Could you also fix formatting? Thanks.

malaskowski · 2018-07-18T13:20:23Z

report/src/main/webapp/app/components/keyboardShortcuts.directive.js

+
+      function callback(mutList) {
+        var finished = false;
+        mutList.forEach(function(mut) {


How about using Array.prototype.find() insead of forEach? You wouldn't have to do this nasty finished guard and finish forEach loop (better performance).

oliwerin · 2018-07-19T12:28:23Z

report/src/main/webapp/app/components/keyboardShortcuts.directive.js

-              testContainer.removeClass('is-active');
-              currentItem.removeClass('is-active');
-              toggleNextTest(suiteContainer);
+                      currentTabLabel = $('.nav-tabs > .nav-item').filter('.active').text().replace(/\s/g, '');


Indentation seems to be out of place in this line

Right, didn't notice. Fixed now.

oliwerin · 2018-07-19T12:30:24Z

report/src/main/webapp/app/components/keyboardShortcuts.directive.js

+
+      function clickOnTab() {
+        var currentTabLabel = userSettingsService.getLastTab();
+        var $nextTestTabs = $('.nav-tabs').children();


In some places you use multiple var notation (here) and in others one var and variables declarations separated out by a comma. I'd be good to stick with just one of them.

Changed, but I still got some stuff to do (with MutationObserver) so I will commit tomorrow.

tkaik · 2018-07-19T13:32:00Z

Please update Changelog file https://github.com/Cognifide/aet/blob/master/CONTRIBUTING.md#changelog

tkaik · 2018-07-27T12:35:10Z

report/src/main/webapp/app/components/keyboardShortcuts.directive.js

-              testContainer.removeClass('is-active');
-              currentItem.removeClass('is-active');
-              toggleNextTest(suiteContainer);
+              currentTabLabel = $('.nav-tabs > .nav-item').filter('.active').text().replace(/\s/g, '');


This line is repeated across the file few times - can it be moved to a seperate function, e.g. getCurrentTabLabel?

Fixed wttech#70 - user now stays in the same tab while navigating aro…

695d7ad

…und the tree

adamkosmala reviewed Jul 18, 2018

View reviewed changes

oliwerin reviewed Jul 18, 2018

View reviewed changes

malaskowski reviewed Jul 18, 2018

View reviewed changes

Marcin Suchorski added 2 commits July 19, 2018 13:55

Fixed wttech#70 - structure and variable names fixes

148b425

Fixed wttech#70 - variable names and structure fixes

123c106

oliwerin reviewed Jul 19, 2018

View reviewed changes

Marcin Suchorski added 2 commits July 20, 2018 09:00

Fixed wttech#70 - minor fixes & optimalization

3625c74

Added information in changelog

f7a2ce7

oliwerin approved these changes Jul 27, 2018

View reviewed changes

tkaik reviewed Jul 27, 2018

View reviewed changes

Minor structure fixes

d97eb26

tkaik approved these changes Jul 27, 2018

View reviewed changes

Merge branch 'master' into feature/same-tab-raport-navigation

830d72e

malaskowski approved these changes Jul 30, 2018

View reviewed changes

malaskowski merged commit 97272c8 into wttech:master Jul 30, 2018

malaskowski added the enhancement label Jul 30, 2018

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fixed #70 - user now stays in the same tab while navigating around th… #289

Fixed #70 - user now stays in the same tab while navigating around th… #289

m-suchorski commented Jul 18, 2018 •

edited by Slasheruus

Loading

adamkosmala Jul 18, 2018

m-suchorski Jul 18, 2018

adamkosmala Jul 18, 2018

oliwerin Jul 18, 2018

m-suchorski Jul 18, 2018

oliwerin Jul 18, 2018

oliwerin Jul 18, 2018

oliwerin Jul 18, 2018

m-suchorski Jul 18, 2018

malaskowski Jul 18, 2018

oliwerin Jul 19, 2018

oliwerin Jul 18, 2018

m-suchorski Jul 18, 2018

malaskowski Jul 18, 2018

oliwerin Jul 18, 2018 •

edited

Loading

m-suchorski Jul 18, 2018

oliwerin Jul 18, 2018

m-suchorski Jul 18, 2018

oliwerin Jul 18, 2018

m-suchorski Jul 18, 2018

oliwerin Jul 19, 2018

malaskowski Jul 18, 2018

malaskowski Jul 18, 2018

malaskowski Jul 18, 2018

malaskowski Jul 18, 2018

oliwerin Jul 19, 2018

m-suchorski Jul 19, 2018

oliwerin Jul 19, 2018 •

edited

Loading

m-suchorski Jul 19, 2018

tkaik commented Jul 19, 2018

tkaik Jul 27, 2018

m-suchorski Jul 27, 2018

		} else {
		currentTab = $('.nav-tabs > .nav-item').filter('.active').text().replace(/\s/g, '');

Fixed #70 - user now stays in the same tab while navigating around th… #289

Fixed #70 - user now stays in the same tab while navigating around th… #289

Conversation

m-suchorski commented Jul 18, 2018 • edited by Slasheruus Loading

Description

Motivation and Context

Types of changes

Checklist:

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

oliwerin Jul 18, 2018 • edited 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

oliwerin Jul 19, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tkaik commented Jul 19, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

m-suchorski commented Jul 18, 2018 •

edited by Slasheruus

Loading

oliwerin Jul 18, 2018 •

edited

Loading

oliwerin Jul 19, 2018 •

edited

Loading