Adding infobar for remote debugging enabled #14956

g-217 · 2019-11-07T12:44:15Z

Adding infobar to tell users, if remote debugging is enabled. This is complementary to brackets-shell PR adobe/brackets-shell#668

swmitra · 2019-11-07T12:53:14Z

@jha-G I guess warning color from bootstrap would suffice instead of error color. Launch the info bar with update mode, first parameter.

g-217 · 2019-11-07T13:15:29Z

This will be the warning color:

swmitra · 2019-11-07T13:21:29Z

Perfect!

shubhsnov · 2019-11-07T20:24:57Z

src/widgets/InfoBar.js

+        });
+    }
+    exports.showUpdateBar = showUpdateBar;
+    exports.RESTART_BTN_CLICKED = RESTART_BTN_CLICKED;


I see this code has been copied from AutoUpdate.

It would be better to trim down this file for just the relevant features required, or better yet make a generic InfoBar that can be reused later. Remove/rename any auto update related stuff.

shubhsnov · 2019-11-07T20:25:42Z

src/htmlContent/infoBar-template.html

@@ -0,0 +1,20 @@
+<div id="update-bar" {{#type}}class={{{type}}}{{/type}} tabindex="0">


Rename auto update related ids to generic ones.

shubhsnov · 2019-11-07T20:26:00Z

src/widgets/InfoBar.js

+                }
+            });
+        }
+        $(window.document).on("keydown.AutoUpdate", function (event) {


Rename event on all other instances.

shubhsnov · 2019-11-07T20:27:46Z

src/brackets.js

+        brackets.app.getRemoteDebuggingPort(function (err, remote_debugging_port){
+            if (remote_debugging_port && remote_debugging_port > 0) {
+                var InfoBar = require('widgets/InfoBar');
+                InfoBar.showUpdateBar({


Again this is context of AutoUpdate. InfoBar.show() would be a better name.

g-217 · 2019-11-08T07:49:14Z

@narayani28 @shubhsnov

swmitra

PR looks OK to me barring some minor comments and questions.

swmitra · 2019-11-11T04:50:28Z

src/styles/images/infobar-checkmarkcircle.svg

+<svg xmlns="http://www.w3.org/2000/svg" viewBox="0 0 16 16">
+  <defs>
+    <style>
+      .cls-1 {
+        fill: #fff;
+      }
+    </style>
+  </defs>
+  <path id="icon-checkmark" class="cls-1" d="M8,16a8,8,0,1,1,8-8A8.009,8.009,0,0,1,8,16ZM3.823,7.833,2.72,9.044l3.071,3.017a1.852,1.852,0,0,0,.919.265,1.463,1.463,0,0,0,1.132-.547c.494-.568,1.131-1.285,1.643-1.861.408-.459.728-.819.77-.873.09-.113,2.462-2.862,2.732-3.175L11.671,4.661c0,.007-.086.1-.222.266C8.679,8.22,7.187,9.976,7.012,10.147c-.084.081-.142.143-.185.188-.115.121-.13.136-.254.151h-.02c-.169,0-.4-.235-.4-.237Z"/>
+</svg>


Where is this being used?

I have retained all three states for InfoBar, Success, Warning, and Error for sake of completion. So infobar-checkmarkcircle.svg will be used if InfoBar.showInfoBar() is called with success state.

swmitra · 2019-11-11T04:50:45Z

src/styles/images/infobar-info.svg

+<svg xmlns="http://www.w3.org/2000/svg" viewBox="0 0 16 16">
+  <defs>
+    <style>
+      .cls-1 {
+        fill: #fff;
+      }
+    </style>
+  </defs>
+  <path id="icon-info" class="cls-1" d="M8,17a8,8,0,1,1,8-8A8.009,8.009,0,0,1,8,17ZM6.91,7.056V14H8.968V7.056ZM7.932,3.892a1.077,1.077,0,1,0,0,2.142,1.077,1.077,0,1,0,0-2.142Z" transform="translate(0 -1)"/>
+</svg>


Info icon background.

swmitra · 2019-11-11T04:51:52Z

src/styles/infobar-styles.less

+/*
+ * Copyright (c) 2019 - present Adobe Systems Incorporated. All rights reserved.
+ *
+ * Permission is hereby granted, free of charge, to any person obtaining a
+ * copy of this software and associated documentation files (the "Software"),
+ * to deal in the Software without restriction, including without limitation
+ * the rights to use, copy, modify, merge, publish, distribute, sublicense,
+ * and/or sell copies of the Software, and to permit persons to whom the
+ * Software is furnished to do so, subject to the following conditions:
+ *
+ * The above copyright notice and this permission notice shall be included in
+ * all copies or substantial portions of the Software.
+ *
+ * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR
+ * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY,
+ * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL THE
+ * AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER
+ * LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING
+ * FROM, OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER
+ * DEALINGS IN THE SOFTWARE.
+ *
+ */
+
+/*info Bar*/
+#info-bar-template {
+    display: block;
+    background-color: #105F9C;


There are few unused selectors. Please clean them up.

Cleaned unused selectors.

swmitra · 2019-11-11T04:52:27Z

src/widgets/infobar.js

+        InfoBarHtml       = require("text!htmlContent/infobar-template.html"),
+        _                  =  require("thirdparty/lodash");
+
+    //addLinkedStyleSheet('styles/infobar-styles.css');


Remove this line.

swmitra · 2019-11-11T04:54:12Z

src/widgets/infobar.js

+        var $infoBar = $('#info-bar-template'),
+            $infoContent = $infoBar.find('#info-content'),
+            $contentContainer = $infoBar.find('#content-container'),
+            $iconContainer = $infoBar.find('#icon-container'),
+            $closeIconContainer = $infoBar.find('#close-icon-container'),
+            $heading = $infoBar.find('#heading'),
+            $description = $infoBar.find('#description'),
+            $closeIcon = $infoBar.find('#close-icon');


Do we need to find all of these UI components for this use case?

Yes, all are used.

swmitra · 2019-11-11T04:54:58Z

src/widgets/infobar.js

+    var MainViewManager     = require("view/MainViewManager"),
+        Mustache            = require("thirdparty/mustache/mustache"),
+        EventDispatcher     = require("utils/EventDispatcher"),
+        InfoBarHtml       = require("text!htmlContent/infobar-template.html"),


NIT: Indentation looks off in this line.

Fixed indentation.

swmitra · 2019-11-11T04:56:25Z

src/widgets/infobar.js

+        };
+
+        resizeContentContainer();
+        $(window).on('resize.AutoInfoBar', _.debounce(resizeContentContainer, 150));


The event name resize.AutoInfoBar doesn't look relevant to Debugger enabled warning!

Renamed the label.

swmitra · 2019-11-11T04:57:03Z

src/widgets/infobar.js

+                MainViewManager.focusActivePane();
+            });
+        }
+        $(window.document).on("keydown.AutoInfo", function (event) {


The event name keydown.AutoInfo doesn't look relevant to Debugger enabled warning!

Renamed the label.

swmitra · 2019-11-11T04:59:55Z

src/htmlContent/infobar-template.html

@@ -0,0 +1,13 @@
+<div id="info-bar-template" {{#type}}class={{{type}}}{{/type}} tabindex="0">


Please test tabindex conflict with actual Auto Update notification bars. Auto update notifications should get priority.

shubhsnov · 2019-11-11T07:50:59Z

src/document/DocumentCommandHandlers.js

-                    // In case of an error
-                    _socket.onerror = result.reject;
-                });
+            brackets.app.getRemoteDebuggingPort(function (err, port){


Just curious as to why they check for function definition here? Do we lazyload some modules?

shubhsnov

Looks good now 👍

swmitra

LGTM 👍

Adding infobar for remote debugging enabled

03d683d

Changing infobar type to warning

41c851a

shubhsnov suggested changes Nov 7, 2019

View reviewed changes

g-217 added 3 commits November 8, 2019 15:59

Removed button related codes

2bdc286

infobar is an independent unit

c977dfe

Changed getRemoteDebuggingPort usage

8677756

swmitra suggested changes Nov 11, 2019

View reviewed changes

Addressing review comments

29c08d2

shubhsnov reviewed Nov 11, 2019

View reviewed changes

shubhsnov approved these changes Nov 11, 2019

View reviewed changes

Updating infobar message

a05057d

swmitra approved these changes Nov 11, 2019

View reviewed changes

shubhsnov merged commit ce2b870 into adobe:master Nov 11, 2019

This was referenced Nov 11, 2019

Remove space between localhost: and port in infobar message #14961

Merged

Merging changes to release branch for 1.14.1 patch release #14971

Merged

		@@ -0,0 +1,20 @@
		<div id="update-bar" {{#type}}class={{{type}}}{{/type}} tabindex="0">

		@@ -0,0 +1,13 @@
		<div id="info-bar-template" {{#type}}class={{{type}}}{{/type}} tabindex="0">

Adding infobar for remote debugging enabled #14956

Adding infobar for remote debugging enabled #14956

Uh oh!

Conversation

g-217 commented Nov 7, 2019

Uh oh!

swmitra commented Nov 7, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

g-217 commented Nov 7, 2019

Uh oh!

swmitra commented Nov 7, 2019

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

g-217 commented Nov 8, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

swmitra left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

shubhsnov left a comment

Choose a reason for hiding this comment

Uh oh!

swmitra left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

swmitra commented Nov 7, 2019 •

edited

Loading

g-217 commented Nov 8, 2019 •

edited

Loading