Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Merged
merged 7 commits into from
Jul 30, 2018
Merged

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

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
695d7ad
Fixed #70 - user now stays in the same tab while navigating around th…
Jul 18, 2018
148b425
Fixed #70 - structure and variable names fixes
Jul 19, 2018
123c106
Fixed #70 - variable names and structure fixes
Jul 19, 2018
3625c74
Fixed #70 - minor fixes & optimalization
Jul 20, 2018
f7a2ce7
Added information in changelog
Jul 27, 2018
d97eb26
Minor structure fixes
Jul 27, 2018
830d72e
Merge branch 'master' into feature/same-tab-raport-navigation
tkaik Jul 27, 2018
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
101 changes: 80 additions & 21 deletions report/src/main/webapp/app/components/keyboardShortcuts.directive.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -124,7 +124,8 @@ define(['angularAMD', 'userSettingsService'], function (angularAMD) {
var currentTest,
currentLocation = window.location.hash,
$nextElement,
$firstElementInTest;
$firstElementInTest,
currentTab;

if (!(ifRootPage(currentLocation, '/url/') || ifRootPage(
currentLocation, '/test/') || ifRootPage(currentLocation,
Expand All @@ -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, '');
Copy link

Choose a reason for hiding this comment

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

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

Copy link
Author

Choose a reason for hiding this comment

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

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

Copy link

Choose a reason for hiding this comment

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

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

testContainer.removeClass('is-active');
currentItem.removeClass('is-active');
toggleNextTest(suiteContainer);
$nextElement = suiteContainer.nextAll(
'.aside-report:not(.is-hidden)').first();
$nextElement.addClass('is-expanded');
$nextElement.children().first().addClass('is-active');
scrollTo($nextElement.find('.is-active'));
toggleNextTestItem(suiteContainer);
userSettingsService.setLastTab(currentTab);
}

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

Choose a reason for hiding this comment

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

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

Copy link
Author

Choose a reason for hiding this comment

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

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

Copy link

Choose a reason for hiding this comment

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

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

Copy link

Choose a reason for hiding this comment

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

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

userSettingsService.setLastTab(currentTab);
nextUrl.click();
scrollTo(nextUrl);
$(testUrlSelector).not(nextUrl).removeClass('is-active');
clickOnTab();
}
} else {
currentTest = findCurrentTest(currentLocation.split('/').pop());
$firstElementInTest = currentTest.find('.url-name:not(.is-hidden)').find(
testUrlSelector).first();
currentTest.addClass('is-expanded');
$firstElementInTest.click();
scrollTo($firstElementInTest);
currentTest = findCurrentTest(currentLocation.split('/').pop());
$firstElementInTest = currentTest.find('.url-name:not(.is-hidden)').find(
testUrlSelector).first();
currentTest.addClass('is-expanded');
$firstElementInTest.click();
scrollTo($firstElementInTest);
}
}

function traverseTreeUp(currentItem, testContainer, suiteContainer,
testUrlSelector) {
var previousTest,
currentLocation = window.location.hash,
$previousElement;
$previousElement,
currentTab;

if (!(ifRootPage(currentLocation, '/url/') || ifRootPage(
currentLocation, '/test/') || ifRootPage(currentLocation,
Expand All @@ -183,14 +189,26 @@ define(['angularAMD', 'userSettingsService'], function (angularAMD) {
'.url-name:not(.is-hidden)').filter(':first').find(
testUrlSelector);
if (_.isEmpty(previousTest)) {
$previousElement = suiteContainer.find('.test-name');
scrollTo($previousElement);
$previousElement.click();
currentItem.parents('.url-name').removeClass('is-active');
suiteContainer.addClass('is-expanded');
if (!_.isEmpty($(suiteContainer).prevAll(
Copy link
Contributor

Choose a reason for hiding this comment

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

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

'.aside-report:not(.is-hidden)').first()
.find('.test-name'))) {
currentTab = $('.nav-tabs > .nav-item').filter('.active').text().replace(/\s/g, '');
Copy link

Choose a reason for hiding this comment

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

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

Copy link
Author

Choose a reason for hiding this comment

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

.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.

Copy link
Contributor

Choose a reason for hiding this comment

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

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

Copy link

Choose a reason for hiding this comment

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

@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.

testContainer.removeClass('is-active');
currentItem.removeClass('is-active');
$previousElement = suiteContainer.prevAll(
'.aside-report:not(.is-hidden)').first();
$previousElement.addClass('is-expanded');
$previousElement.children().first().addClass('is-active');
scrollTo($previousElement.find('.is-active'));
togglePrevTestItem(suiteContainer);
userSettingsService.setLastTab(currentTab);
}
} else {
currentTab = $('.nav-tabs > .nav-item').filter('.active').text().replace(/\s/g, '');
userSettingsService.setLastTab(currentTab);
scrollTo(previousTest);
previousTest.click();
clickOnTab();
}
} else {
previousTest = findPreviousTest(currentLocation.split('/').pop());
Expand All @@ -208,13 +226,54 @@ define(['angularAMD', 'userSettingsService'], function (angularAMD) {
}
}

function toggleNextTest(currentTest) {
$(currentTest).nextAll('.aside-report:not(.is-hidden)')
.first()
.find('.test-name')
.click();
function toggleNextTestItem(currentTest) {
$(currentTest).nextAll(
'.aside-report:not(.is-hidden)')
.first()
.find('.test-url')
.first()
.click();
}

function togglePrevTestItem(currentTest) {
$(currentTest).prevAll(
'.aside-report:not(.is-hidden)')
.first()
.find('.test-url')
.last()
.click();
}

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

@oliwerin oliwerin Jul 18, 2018 •
edited
Loading

Choose a reason for hiding this comment

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

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

Copy link
Author

Choose a reason for hiding this comment

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

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

for(var i=0;i <nextTestTabs.length; i++) {
Copy link

Choose a reason for hiding this comment

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

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

Copy link
Author

Choose a reason for hiding this comment

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

I will change that.

Copy link
Contributor

Choose a reason for hiding this comment

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

Could you also fix formatting? Thanks.

if($(nextTestTabs[i]).text().replace(/\s/g, '') === currentTab) {
$(nextTestTabs[i]).find('a').click();
currentTab = null;
}
}
}

var mutationObserver = new MutationObserver(callback);
Copy link

Choose a reason for hiding this comment

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

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

Copy link
Author

Choose a reason for hiding this comment

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

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.

Copy link

Choose a reason for hiding this comment

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

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

Copy link
Author

Choose a reason for hiding this comment

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

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.

Copy link

Choose a reason for hiding this comment

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

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.


function callback(mutList) {
var finished = false;
mutList.forEach(function(mut) {
Copy link
Contributor

Choose a reason for hiding this comment

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

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).

if($(mut.target).hasClass('nav-tabs') && !finished) {
clickOnTab();
finished = true;
}
});
}

mutationObserver.observe(document, {
attributes: true,
characterData: true,
childList: true,
subtree: true,
});

function ifRootPage(url, type) {
return url.search(type) > 0;
}
Expand Down Expand Up @@ -252,4 +311,4 @@ define(['angularAMD', 'userSettingsService'], function (angularAMD) {
}
}
}
});
});
18 changes: 16 additions & 2 deletions report/src/main/webapp/app/services/userSettings.service.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -28,13 +28,17 @@ define(['angularAMD', 'localStorageService'], function (angularAMD) {
isScreenshotMaskVisible: isScreenshotMaskVisible,
toggleScreenshotMask: toggleScreenshotMask,
isFullSourceVisible: isFullSourceVisible,
toggleFullSource: toggleFullSource
toggleFullSource: toggleFullSource,
setLastTab: setLastTab,
getLastTab: getLastTab,
},
SCREENSHOT_MASK_STORAGE_KEY = 'aet:settings.visibility.screenshotMask',
FULL_SOURCE_MASK_STORAGE_KEY = 'aet:settings.visibility.fullSource',
USER_TAB_MASK_STORAGE_KEY = 'aet:settings.raport.lastTab',
settings = {
screenshotMaskVisible: true,
fullSourceMaskVisible: false
fullSourceMaskVisible: false,
lastUserTab: null,
};

setupService();
Expand All @@ -45,6 +49,8 @@ define(['angularAMD', 'localStorageService'], function (angularAMD) {
true);
settings.fullSourceMaskVisible = getSetting(FULL_SOURCE_MASK_STORAGE_KEY,
false);
settings.lastUserTab = getSetting(USER_TAB_MASK_STORAGE_KEY,
null);
}

function isScreenshotMaskVisible() {
Expand All @@ -67,6 +73,14 @@ define(['angularAMD', 'localStorageService'], function (angularAMD) {
return settings.fullSourceMaskVisible;
}

function setLastTab(val) {
settings.userTab = val;
}

function getLastTab() {
return settings.userTab;
}

/***************************************
*********** Private methods *********
***************************************/
Expand Down