Skip to content
This repository has been archived by the owner on Sep 6, 2021. It is now read-only.

#7276 Live Preview highlight customization #12949

Merged
merged 14 commits into from
Apr 6, 2017
Merged

#7276 Live Preview highlight customization #12949

Show file tree
Hide file tree
Changes from 13 commits
Commits
Show all changes
14 commits
Select commit Hold shift + click to select a range
b1ef3be
Live Preview visual customization (colors of highlight can be customi…
Worie Nov 27, 2016
cb9fb57
PR improvements
Worie Nov 28, 2016
6dfd2de
Allow to animate transform property via transition
Worie Nov 28, 2016
5250924
Remove old vendor prefixes from transitionValues in RemoteFunctions.js
Worie Nov 28, 2016
0e014c5
updateConfig RemoteFunctions.js method
Worie Nov 29, 2016
6d13f11
Show margin and paddings in live preview.
Worie Nov 30, 2016
3f2d984
Do not show element if it has width or height equal to 0. A little bi…
Worie Dec 2, 2016
0889f8f
PR improvements
Worie Dec 9, 2016
108faeb
Redraw via requestAnimationFrame if element has transition / animatio…
Worie Dec 19, 2016
004afc6
PR improvements.
Worie Dec 19, 2016
ae19b23
Fix ESLINT errors.
Worie Dec 19, 2016
36c356e
drawMarginRect and drawPaddingRect functions now calculate the requir…
Worie Jan 28, 2017
24b2e6b
Temporary workaround for transform issue.
Worie Feb 15, 2017
12b882d
Formatting improvements
Mar 24, 2017
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
2 changes: 1 addition & 1 deletion src/LiveDevelopment/Agents/RemoteAgent.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -131,7 +131,7 @@ define(function RemoteAgent(require, exports, module) {
_stopKeepAliveInterval();

// inject RemoteFunctions
var command = "window._LD=" + RemoteFunctions + "(" + LiveDevelopment.config.experimental + "," + PreferencesManager.get("livedev.wsPort") + ");";
var command = "window._LD=" + RemoteFunctions + "(" + JSON.stringify(LiveDevelopment.config) + "," + PreferencesManager.get("livedev.wsPort") + ");";

Inspector.Runtime.evaluate(command, function onEvaluate(response) {
if (response.error || response.wasThrown) {
Expand Down
191 changes: 171 additions & 20 deletions src/LiveDevelopment/Agents/RemoteFunctions.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -30,10 +30,29 @@
* modules should define a single function that returns an object of all
* exported functions.
*/
function RemoteFunctions(experimental, remoteWSPort) {
function RemoteFunctions(config, remoteWSPort) {
Copy link
Contributor

Choose a reason for hiding this comment

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

I think we can pass remoteWSPort as part of config only, something like config.remoteWSPort.
Then instead of checking for remoteWSPort in https://github.com/adobe/brackets/pull/12949/files#diff-a40df952af8fd1808c321dab548ae11aR1032, we can check for !experimental flag as we create websocket only when we are not using experimental live preview.
But I think you can leave this for later as you might have to make changes which are not relevant for this feature, once this is merged I can make the above changes.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hi, you're right. Unfortunetely I won't be accessible for a while, so I guess we could close this one and open new PRs as necessary, if that's OK.

"use strict";

var experimental;
if (!config) {
experimental = false;
} else {
experimental = config.experimental;
}
var lastKeepAliveTime = Date.now();
var req, timeout;
Copy link
Collaborator

Choose a reason for hiding this comment

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

This commit has tabs, while we prefer/enforce 4 spaces. You can easily change them with the brackets-eslint extension by right clicking the editor and selecting Autofix with ESLint

var animateHighlight = function (time) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

L44-L55 have some indentation issues, convert tabs -> spaces and they should be all right

if(req) {
window.cancelAnimationFrame(req);
window.clearTimeout(timeout);
}
req = window.requestAnimationFrame(redrawHighlights);

timeout = setTimeout(function () {
window.cancelAnimationFrame(req);
req = null;
}, time * 1000);
};

/**
* @type {DOMEditHandler}
Expand Down Expand Up @@ -250,17 +269,149 @@ function RemoteFunctions(experimental, remoteWSPort) {
}
return false;
},

_makeHighlightDiv: function (element, doAnimation) {
var elementBounds = element.getBoundingClientRect(),
highlight = window.document.createElement("div"),
styles = window.getComputedStyle(element);
elementStyling = window.getComputedStyle(element),
transitionDuration = parseFloat(elementStyling.getPropertyValue('transition-duration')),
animationDuration = parseFloat(elementStyling.getPropertyValue('animation-duration'));

if (transitionDuration) {
animateHighlight(transitionDuration);
}

if (animationDuration) {
animateHighlight(animationDuration);
}

// Don't highlight elements with 0 width & height
if (elementBounds.width === 0 && elementBounds.height === 0) {
return;
}

var realElBorder = {
"right": elementStyling.getPropertyValue('border-right-width'),
Copy link
Collaborator

Choose a reason for hiding this comment

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

No need to quote the keys in L293-L296

"left": elementStyling.getPropertyValue('border-left-width'),
"top": elementStyling.getPropertyValue('border-top-width'),
"bottom": elementStyling.getPropertyValue('border-bottom-width'),
};

var innerWidth = elementBounds.width - parseInt(realElBorder.left) - parseInt(realElBorder.right);
var innerHeight = elementBounds.height - parseInt(realElBorder.top) - parseInt(realElBorder.bottom);

var visualisations = [];
Copy link
Collaborator

Choose a reason for hiding this comment

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

This should be an object literal instead of assigning horizontal and vertical as array properties?

var visualisations = {
 horizontal: "left, right",
 vertical: "top, bottom",
}

visualisations['horizontal'] = "left, right";
visualisations['vertical'] = "top, bottom";

var drawPaddingRect = function(side) {
var elStyling = {};

if(visualisations['horizontal'].indexOf(side) >= 0) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Maybe prefer the dot-notation (visualisations.horizontal etc.) in L309-351.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Nit: space between if and ( -> if (visualisations)...

elStyling['width'] = elementStyling.getPropertyValue('padding-' + side);
elStyling['height'] = innerHeight + "px";
elStyling['top'] = realElBorder.top;

} else {
elStyling['height'] = elementStyling.getPropertyValue('padding-' + side);
elStyling['width'] = innerWidth + "px";
elStyling['left'] = realElBorder.left;
}

elStyling[side] = realElBorder[side];
elStyling['position'] = 'absolute';

return elStyling;
};

var drawMarginRect = function(side) {
var elStyling = {};

var margin = [];
Copy link
Collaborator

Choose a reason for hiding this comment

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

Nit: extra space between var and margin

margin['right'] = parseInt(elementStyling.getPropertyValue('margin-right'));
margin['top'] = parseInt(elementStyling.getPropertyValue('margin-top'));
margin['bottom'] = parseInt(elementStyling.getPropertyValue('margin-bottom'));
margin['left'] = parseInt(elementStyling.getPropertyValue('margin-left'));


if(visualisations['horizontal'].indexOf(side) >= 0) {
elStyling['width'] = elementStyling.getPropertyValue('margin-' + side);
elStyling['height'] = elementBounds.height + margin['top'] + margin['bottom'] + "px";
elStyling['top'] = "-" + margin['top'] + "px";

} else {
elStyling['height'] = elementStyling.getPropertyValue('margin-' + side);
elStyling['width'] = elementBounds.width + "px";
elStyling['left'] = 0;
}

elStyling[side] = "-" + margin[side] + "px";
elStyling['position'] = 'absolute';

return elStyling;
};

var setVisibility = function (el) {
if (
!config.remoteHighlight.showPaddingMargin ||
parseInt(el.height, 10) <= 0 ||
parseInt(el.width, 10) <= 0
) {
el.display = 'none';
} else {
el.display = 'block';
}
};

var mainBoxStyles = config.remoteHighlight.stylesToSet;
mainBoxStyles['border'] = 'none';
Copy link
Collaborator

Choose a reason for hiding this comment

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

Nit: mainBoxStyles should be aligned to var


var paddingVisualisations = [
Copy link
Collaborator

Choose a reason for hiding this comment

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

Not sure if these would be better in their own file, for example RemoteVisualizationUtils for example?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hmm, but RemoteFunctions are being injected into the browser (window._LD) and those values are calculated basing on the element that is highlighted there... I'm not sure how that sould work - how to access another file from there? Can't imagine this atm.

Or perhaps i simply didn't understand what you meant :)

Copy link
Collaborator

Choose a reason for hiding this comment

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

Yeah my bad, I was thinking about making it an utility file of methods that get passed to that injected method but it most likely won't work like that nor would be worth it. Feel free to disregard 👍

drawPaddingRect('top'),
drawPaddingRect('right'),
drawPaddingRect('bottom'),
drawPaddingRect('left')
];

var marginVisualisations = [
drawMarginRect('top'),
drawMarginRect('right'),
drawMarginRect('bottom'),
drawMarginRect('left')
];

var setupVisualisations = function (arr, config) {
var i;
for (i = 0; i < arr.length; i++) {
setVisibility(arr[i]);

// Applies to every visualisationElement (padding or margin div)
arr[i]["box-sizing"] = "border-box";
arr[i]["transform"] = "none";
var el = window.document.createElement("div"),
styles = Object.assign(
{},
config,
arr[i]
);

_setStyleValues(
Copy link
Collaborator

Choose a reason for hiding this comment

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

Nit: This should be a one-liner as there's only couple of short properties

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Oh, okay. It's common practice for me to place parameters (even short ones) in separate lines so git displays the changes in more friendly way. But sure, I'll make it a one-liner. 👍

styles,
el.style
);

highlight.appendChild(el);
}
};

setupVisualisations(
marginVisualisations,
config.remoteHighlight.marginStyling
);
setupVisualisations(
paddingVisualisations,
config.remoteHighlight.paddingStyling
);

highlight.className = HIGHLIGHT_CLASSNAME;

var offset = _screenOffset(element);
Expand All @@ -275,31 +426,25 @@ function RemoteFunctions(experimental, remoteWSPort) {
"padding": 0,
"position": "absolute",
"pointer-events": "none",
"border-top-left-radius": styles.borderTopLeftRadius,
"border-top-right-radius": styles.borderTopRightRadius,
"border-bottom-left-radius": styles.borderBottomLeftRadius,
"border-bottom-right-radius": styles.borderBottomRightRadius,
"border-top-left-radius": elementStyling.borderTopLeftRadius,
"border-top-right-radius": elementStyling.borderTopRightRadius,
"border-bottom-left-radius": elementStyling.borderBottomLeftRadius,
"border-bottom-right-radius": elementStyling.borderBottomRightRadius,
"border-style": "solid",
"border-width": "1px",
"border-color": "#00a2ff",
"box-shadow": "0 0 1px #fff",
"box-sizing": "border-box"
};

var mergedStyles = Object.assign({}, stylesToSet, config.remoteHighlight.stylesToSet);
Copy link
Collaborator

@ficristo ficristo Nov 29, 2016 •
edited
Loading

Choose a reason for hiding this comment

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

I think Object.assign is not available on Linux (which uses Chrome 29). You could try to use lodash assign or extend instead.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Argh, you are right @ficristo. The age old Linux version is starting to become the IE8 of Brackets 😭

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Those are remote functions, so I cannot use _.assign there I guess until i inject it to the browser. Am I right?

I've left it as Object.assign, just for now - until I know what exactly am I supposed to do.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Actually as they are remote functions that are run on the browser, we can just use Object.assign right?

Copy link
Collaborator

Choose a reason for hiding this comment

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

You mean a real browser and not Brackets itself, right? (Not much familiar with LivePreview)
Then I suppose we can use Object.assign. Sorry for the noise...

Copy link
Contributor Author

@Worie Worie Nov 30, 2016 •
edited
Loading

Choose a reason for hiding this comment

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

I cant test it on Linux, but it works well on OSX. Edit: Yeah, those are run in real browser.


var animateStartValues = {
"background-color": "rgba(0, 162, 255, 0.5)",
"opacity": 0
};
var animateStartValues = config.remoteHighlight.animateStartValue;

var animateEndValues = {
"background-color": "rgba(0, 162, 255, 0)",
"opacity": 1
};
var animateEndValues = config.remoteHighlight.animateEndValue;

var transitionValues = {
"-webkit-transition-property": "opacity, background-color",
"-webkit-transition-duration": "300ms, 2.3s",
"transition-property": "opacity, background-color",
"transition-property": "opacity, background-color, transform",
"transition-duration": "300ms, 2.3s"
};

Expand All @@ -311,7 +456,7 @@ function RemoteFunctions(experimental, remoteWSPort) {
}
}

_setStyleValues(stylesToSet, highlight.style);
_setStyleValues(mergedStyles, highlight.style);
_setStyleValues(
doAnimation ? animateStartValues : animateEndValues,
highlight.style
Expand Down Expand Up @@ -842,6 +987,11 @@ function RemoteFunctions(experimental, remoteWSPort) {
function getSimpleDOM() {
return JSON.stringify(_domElementToJSON(window.document.documentElement));
}

function updateConfig(newConfig) {
config = JSON.parse(newConfig);
return JSON.stringify(config);
}

// init
_editHandler = new DOMEditHandler(window.document);
Expand Down Expand Up @@ -894,6 +1044,7 @@ function RemoteFunctions(experimental, remoteWSPort) {
"highlightRule" : highlightRule,
"redrawHighlights" : redrawHighlights,
"applyDOMEdits" : applyDOMEdits,
"getSimpleDOM" : getSimpleDOM
"getSimpleDOM" : getSimpleDOM,
"updateConfig" : updateConfig
};
}
37 changes: 37 additions & 0 deletions src/LiveDevelopment/main.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -79,6 +79,33 @@ define(function main(require, exports, module) {
description: Strings.DESCRIPTION_LIVE_DEV_MULTIBROWSER
});

// "livedev.remoteHighlight" preference
var PREF_REMOTEHIGHLIGHT = "remoteHighlight";
var remoteHighlightPref = prefs.definePreference(PREF_REMOTEHIGHLIGHT, "object", {
animateStartValue: {
"background-color": "rgba(0, 162, 255, 0.5)",
"opacity": 0
},
animateEndValue: {
"background-color": "rgba(0, 162, 255, 0)",
"opacity": 0.6
},
"paddingStyling": {
"border-width": "1px",
"border-style": "dashed",
"border-color": "rgba(0, 162, 255, 0.5)"
},
"marginStyling": {
"background-color": "rgba(21, 165, 255, 0.58)"
},
"stylesToSet": {
"border-width": "1px"
},
"showPaddingMargin": true
}, {
description: "LivePreview highlight settings"
});

/** Toggles or sets the preference **/
function _togglePref(key, value) {
var val,
Expand Down Expand Up @@ -302,6 +329,7 @@ define(function main(require, exports, module) {
/** Initialize LiveDevelopment */
AppInit.appReady(function () {
params.parse();
config.remoteHighlight = prefs.get(PREF_REMOTEHIGHLIGHT);

Inspector.init(config);
LiveDevelopment.init(config);
Expand Down Expand Up @@ -353,6 +381,15 @@ define(function main(require, exports, module) {
_setImplementation(prefs.get(PREF_MULTIBROWSER));
}
});

remoteHighlightPref
.on("change", function () {
config.remoteHighlight = prefs.get(PREF_REMOTEHIGHLIGHT);

if (LiveDevImpl && LiveDevImpl.status >= LiveDevImpl.STATUS_ACTIVE) {
LiveDevImpl.agents.remote.call("updateConfig",JSON.stringify(config));
}
});

});

Expand Down