Advanced screen comparison feature #297

m-suchorski · 2018-07-30T09:10:18Z

More advanced way of comparing screen layouts.

Description

This feature gives user a function to compare screen layouts in more advanced way. User can manually start the comparison.

I've tried to make the original code more readable.

Author: @aux-lux

Motivation and Context

Fixes for this PR: #269

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.

adamkosmala · 2018-07-30T11:04:16Z

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

+  'use strict';
+  angularAMD.directive('aetCompareScreens', compareScreensDirective);
+
+


please remove empty lines

Sorry for that, they should all be removed by now.

adamkosmala · 2018-07-30T11:04:51Z

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

+
+  function linkFunc(scope) {
+
+    scope.advancedScreenComparison = function () {


it's a good practice not to keep empty line after function declaration

adamkosmala · 2018-07-30T11:05:45Z

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

+      var tab = document.querySelector('.test-tabs > .tab-content > .tab-pane.ng-scope.active');
+      var wrapper = tab.querySelector('.page-main .layout-compare');
+      var initedClass = 'screen-comparison-active';
+      var arrangeTimeout;


please assign initial value for this variable, in order to allocate it properly in memory

adamkosmala · 2018-07-30T11:06:57Z

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

+      var group = [];
+
+
+


empty lines

adamkosmala · 2018-07-30T11:08:05Z

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

+      canvas.canvasWrapper.appendChild(canvas.canvasB);
+
+
+


empty lines

adamkosmala · 2018-07-30T11:17:50Z

report/src/main/webapp/assets/sass/_newComparison.scss

+      max-width: 48%;
+      vertical-align: top;
+      box-sizing: content-box;
+      &+canvas {


& is not needed in this case, simple + will do

adamkosmala · 2018-07-30T11:18:15Z

report/src/main/webapp/assets/sass/_test.scss

@@ -40,19 +40,25 @@
    font-size: 11px;
    display: inline-block;
    padding: 0 20px;
-    line-height: 30px;
+    line-height: 31px;


if line-height is declared, you do not need setting height

To be honest I don't know why this value is even modified. Deleted height value anyway.

adamkosmala · 2018-07-30T11:19:40Z

report/src/main/webapp/app/layout/main/url/reports/screen_layout.html

@@ -29,6 +29,9 @@
      </label>
    </div>
  </div>
+  <div class="try-new button button-small button-blue">
+  <a ng-click="advancedScreenComparison()" aet-compare-screens>Advanced Screen Comparison</a>


please use data prefix for custom attributes- same goes for all directives, it should be data-ng-click and data-aet-compare-screens

Didn't know that, thanks. Done now.

adamkosmala · 2018-07-30T11:21:52Z

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

+    }
+
+    function deselectGroups(label, ruler, wrapper) {
+      label.labelA.style.display = 'none';


consider manipulating styles with class on the parent element- otherwise this code will trigger four sequential repaint events (same goes for adding block display)

I've changed my approach to this one, please take a look and tell me if it's all right now.

adamkosmala · 2018-07-30T11:24:50Z

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

+      var firstGroup = [];
+      var secondGroup = [];
+      if (groups[0][0] > 0) {
+        for (var i = 0; i < groups[0][0]; i++) {


you create multiple variable declarations with the same name- in each variable, within the same function scope, they should be unique- think about hoisting ;)

malaskowski

Please update CHANGELOG according to AET Contributing rules

malaskowski

Please update Layout report documentation with description of this new feature (and maybe one example screenshot?).

malaskowski · 2018-08-01T13:23:52Z

report/src/main/webapp/app/layout/main/url/reports/screen_layout.html

@@ -29,6 +29,9 @@
      </label>
    </div>
  </div>
+  <div class="try-new button button-small button-blue">


Could you please use more descriptive class name here?
E.g. advanced-screen-comparison or smth like that

malaskowski · 2018-08-01T13:27:12Z

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

+        invertedGroupsB: [],
+      };
+
+      var groups = [];


Could you please make those variables more meaningful?

malaskowski · 2018-08-01T13:27:52Z

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

+        imgSizeB: null,
+      };
+
+      var simple = {


What is simple?

malaskowski · 2018-08-01T13:31:17Z

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

+        wrapper.classList.add(initedClass);
+      }
+
+      var button = document.querySelector('.try-new');


Could you please encapsulate logic in the lines below into a function, e.g. markComparisonStarted or smth. similar?

malaskowski · 2018-08-01T13:31:39Z

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

+      loadImage(img.imgB, imgSize.imgSizeB, canvas.canvasB, context.contextB, simple.simpleB, function (returnedSimple, returnedImgSize) {
+        simple.simpleB = returnedSimple;
+        imgSize.imgSizeB = returnedImgSize;
+        button.classList.remove('button-disabled');


Could you please encapsulate logic in the lines below into a function, e.g. markComparisonFinished or smth. similar?

I've actually left it how it were mainly because I'd need to pass more than 15 parameters to this new function. I might think of something else in the future.

malaskowski · 2018-08-01T13:32:45Z

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

+    function drawDifferences(difs, imgSize, invertedGroups, simple, context) {
+      var maxHeight = Math.max(imgSize.imgSizeA.height, imgSize.imgSizeB.height);
+      var proc = 100 / maxHeight;
+      var newDiffBLeft = '52%';


What does this magic number mean?

It's the left value of second canvas' differences that's being created few lines below. I got no idea how to make it clearer, that second canvas just needs to be pushed by 52% from the left to make both canvases apprear next to each other.

…om/m-suchorski/aet into feature/advanced-screen-comparison

Marcin Suchorski added 4 commits July 27, 2018 08:57

Advanced screen comparison feature

c18dd3f

Minor JS structure fixes

33adea4

Added information about process being in progress

fb34aee

Added license

e25b0c8

adamkosmala suggested changes Jul 30, 2018

View reviewed changes

Marcin Suchorski added 2 commits July 30, 2018 14:26

Minor styling and JS fixes

15b2b87

Added data prefix for custom attributes

83a2a38

malaskowski reviewed Aug 1, 2018

View reviewed changes

malaskowski suggested changes Aug 1, 2018

View reviewed changes

malaskowski added the enhancement label Aug 1, 2018

JS & SASS fixes

61cb5f0

malaskowski approved these changes Aug 3, 2018

View reviewed changes

Marcin Suchorski and others added 5 commits August 28, 2018 10:53

Comparison button is now visible only on failed tests

1f8ec11

Merge branch 'master' into feature/advanced-screen-comparison

c1edc6d

Button is now visible for conditionally passed tests

09ca3da

Merge branch 'feature/advanced-screen-comparison' of https://github.c…

615ab73

…om/m-suchorski/aet into feature/advanced-screen-comparison

Merge branch 'master' into feature/advanced-screen-comparison

596df4b

tkaik merged commit c8ec6ef into wttech:master Aug 29, 2018

malaskowski mentioned this pull request Aug 29, 2018

Adding new feature - advanced screen comparison. #269

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Advanced screen comparison feature #297

Advanced screen comparison feature #297

m-suchorski commented Jul 30, 2018

adamkosmala Jul 30, 2018

m-suchorski Jul 30, 2018

adamkosmala Jul 30, 2018

m-suchorski Jul 30, 2018

adamkosmala Jul 30, 2018

m-suchorski Jul 30, 2018

adamkosmala Jul 30, 2018

adamkosmala Jul 30, 2018

adamkosmala Jul 30, 2018

adamkosmala Jul 30, 2018

m-suchorski Jul 30, 2018

adamkosmala Jul 30, 2018

m-suchorski Jul 30, 2018

adamkosmala Jul 30, 2018

m-suchorski Jul 30, 2018

adamkosmala Jul 30, 2018

m-suchorski Jul 30, 2018

malaskowski left a comment

malaskowski left a comment

malaskowski Aug 1, 2018

m-suchorski Aug 2, 2018

malaskowski Aug 1, 2018

m-suchorski Aug 2, 2018

malaskowski Aug 1, 2018

m-suchorski Aug 2, 2018

malaskowski Aug 1, 2018

m-suchorski Aug 2, 2018

malaskowski Aug 1, 2018

m-suchorski Aug 2, 2018

malaskowski Aug 1, 2018

m-suchorski Aug 2, 2018

		'use strict';
		angularAMD.directive('aetCompareScreens', compareScreensDirective);


		function linkFunc(scope) {

		scope.advancedScreenComparison = function () {

Advanced screen comparison feature #297

Advanced screen comparison feature #297

Conversation

m-suchorski commented Jul 30, 2018

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

malaskowski left a comment

Choose a reason for hiding this comment

malaskowski left a 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