Use the unified console style for WebMKS consoles #2662

bmclaughlin · 2017-11-07T20:43:04Z

Makes the appearance of the WebMKS console consistent with our other consoles as well as simplifying access to full screen and ctl-alt-del button sequence for the user.

Before:

After:

@miq-bot add_labels bug, compute/infrastructure

https://bugzilla.redhat.com/show_bug.cgi?id=1490640

bmclaughlin · 2017-11-07T21:24:20Z

The failing codeclimate checks are in code extracted to an included file and the failed checks are intended (browser specific function calls with similar names, the default switch statement is first followed by browser specific cases).

mzazrivec · 2017-11-09T09:54:18Z

@skateman review please?

skateman

The remote consoles in general are using a fully custom view, i.e. without the application.html.haml on purpose. We don't want to load all the unnecessary JS that powers our angular-based stuff in a popup that's totally isolated.

skateman · 2017-11-09T09:51:45Z

app/views/vm_common/console_webmks.html.haml

-    = stylesheet_link_tag 'webmks'
-    = javascript_include_tag 'jquery/dist/jquery.js', 'jquery-ui/jquery-ui.js', 'webmks'
+    = stylesheet_link_tag 'application', 'webmks'
+    = javascript_include_tag 'application', 'webmks'


Why would you want to include our whole JS codebase for remote consoles?

Same as above:

= javascript_include_tag 'jquery/dist/jquery.js', 'jquery-ui/jquery-ui.js', 'webmks', 'remote_console'

skateman · 2017-11-09T16:18:28Z

app/assets/javascripts/application.js

@@ -78,6 +78,7 @@
 //= require miq_explorer
 //= require qs
 //= require miq_timeline
+//= require remote_consoles/fullscreen_btn


Remove this line please, we don't want this in the global JS

skateman · 2017-11-09T16:19:37Z

app/assets/javascripts/remote_consoles/fullscreen_btn.js

@@ -0,0 +1,29 @@
+//= require jquery


You don't need the jquery here, we're including it explicitly later. Also rename this file to something more "generic", like remote_console.js and move it one folder up.

skateman · 2017-11-09T16:20:07Z

app/assets/stylesheets/remote_consoles/unified_styles.scss

@@ -0,0 +1,16 @@
+#remote-console {


As with the JS, rename this file and move it up...

skateman · 2017-11-09T16:20:44Z

app/views/layouts/remote_console.html.haml

-        });
-
-      });
+    = javascript_include_tag 'application', "remote_consoles/#{@console[:type]}"


= javascript_include_tag 'jquery/dist/jquery.js', "remote_consoles/#{@console[:type]}", "remote_console"

skateman · 2017-11-09T16:22:17Z

app/views/shared/_consoles_unified_look_and_feel.html.haml

@@ -0,0 +1,9 @@
+%footer


Can this be also something more "generic"? Like remote_console_footer?

skateman · 2017-11-09T16:22:49Z

app/views/vm_common/console_webmks.html.haml

@@ -4,16 +4,23 @@
    %title
      = _('%{product_name} WebMKS Remote Console') % {:product_name => I18n.t('product.name')}
    = favicon_link_tag
-    = stylesheet_link_tag 'webmks'
-    = javascript_include_tag 'jquery/dist/jquery.js', 'jquery-ui/jquery-ui.js', 'webmks'
+    = stylesheet_link_tag 'application', 'webmks'


That's fine, CSS is OK to be global.

skateman · 2017-11-09T16:23:41Z

app/views/vm_common/console_webmks.html.haml

-    = stylesheet_link_tag 'webmks'
-    = javascript_include_tag 'jquery/dist/jquery.js', 'jquery-ui/jquery-ui.js', 'webmks'
+    = stylesheet_link_tag 'application', 'webmks'
+    = javascript_include_tag 'application', 'webmks'


Same as above:

= javascript_include_tag 'jquery/dist/jquery.js', 'jquery-ui/jquery-ui.js', 'webmks', 'remote_console'

skateman · 2017-11-09T16:26:20Z

config/initializers/assets.rb

@@ -18,4 +18,6 @@
  spice-html5-bower/spiceHTML5/spicearraybuffer.js
  webmks.css
  webmks.js
+  remote_consoles/unified_styles.css
+  remote_consoles/fullscreen_btn.js


These lines make sure that the assets are reachable from javascript_include_tag and stylesheet_link_tag. You don't need the CSS as we're having this through application.css.

skateman · 2017-11-09T18:36:35Z

app/assets/javascripts/application.js

@@ -78,6 +78,7 @@
 //= require miq_explorer
 //= require qs
 //= require miq_timeline
+//= require remote_consoles/remote_console


you don't need this if you don't include the application

good catch, thanks!

skateman · 2017-11-09T18:49:14Z

app/assets/javascripts/remote_consoles/remote_console.js

@@ -0,0 +1,29 @@
+//= require jquery


you don't need this, and move the file up by one folder, I won't expect any other JS code here

miq-bot · 2017-11-10T08:23:40Z

This pull request is not mergeable. Please rebase and repush.

skateman · 2017-11-10T19:17:58Z

app/assets/stylesheets/remote_consoles/remote_console.scss

@@ -0,0 +1,16 @@
+#remote-console {


the name of this file 😉 please 🐶 a folder up

https://bugzilla.redhat.com/show_bug.cgi?id=1490640

miq-bot · 2017-11-13T15:02:22Z

Checked commit bmclaughlin@c4814f6 with ruby 2.3.3, rubocop 0.47.1, and haml-lint 0.20.0
1 file checked, 0 offenses detected
Everything looks fine. 🍪

skateman · 2017-11-13T15:11:56Z

@bmclaughlin looks good, will test it against all console types

skateman

…ks-consoles Use the unified console style for WebMKS consoles (cherry picked from commit eb1c41a) https://bugzilla.redhat.com/show_bug.cgi?id=1515415

simaishi · 2017-11-20T20:01:10Z

Gaprindashvili backport details:

$ git log -1
commit 65cedc39a004d8c3cdd92ce42052f39f30589816
Author: Milan Zázrivec <mzazrivec@redhat.com>
Date:   Mon Nov 20 09:32:51 2017 +0100

    Merge pull request #2662 from bmclaughlin/ctl-alt-del-button-for-webmks-consoles
    
    Use the unified console style for WebMKS consoles
    (cherry picked from commit eb1c41aa5c6cc9acd514171bee2db36799ae0fce)
    
    https://bugzilla.redhat.com/show_bug.cgi?id=1515415

…ks-consoles Use the unified console style for WebMKS consoles (cherry picked from commit eb1c41a) https://bugzilla.redhat.com/show_bug.cgi?id=1515416

simaishi · 2017-11-28T19:57:12Z

Fine backport details:

$ git log -1
commit dfe70c0e7436b6b057bc5d8faea60f8015505ca8
Author: Milan Zázrivec <mzazrivec@redhat.com>
Date:   Mon Nov 20 09:32:51 2017 +0100

    Merge pull request #2662 from bmclaughlin/ctl-alt-del-button-for-webmks-consoles
    
    Use the unified console style for WebMKS consoles
    (cherry picked from commit eb1c41aa5c6cc9acd514171bee2db36799ae0fce)
    
    https://bugzilla.redhat.com/show_bug.cgi?id=1515416

miq-bot added bug compute/infrastructure labels Nov 7, 2017

skateman suggested changes Nov 9, 2017

View reviewed changes

bmclaughlin force-pushed the ctl-alt-del-button-for-webmks-consoles branch 2 times, most recently from e03ed6b to 8e5662c Compare November 9, 2017 18:23

skateman reviewed Nov 9, 2017

View reviewed changes

bmclaughlin force-pushed the ctl-alt-del-button-for-webmks-consoles branch from 8e5662c to cd894e3 Compare November 9, 2017 18:47

skateman reviewed Nov 9, 2017

View reviewed changes

bmclaughlin force-pushed the ctl-alt-del-button-for-webmks-consoles branch 3 times, most recently from a33e8f6 to 15da5f9 Compare November 9, 2017 19:28

miq-bot added the unmergeable label Nov 10, 2017

bmclaughlin force-pushed the ctl-alt-del-button-for-webmks-consoles branch from 15da5f9 to 213fbf2 Compare November 10, 2017 17:57

miq-bot removed the unmergeable label Nov 10, 2017

skateman reviewed Nov 10, 2017

View reviewed changes

Use the unified console styles for WebMKS consoles

c4814f6

https://bugzilla.redhat.com/show_bug.cgi?id=1490640

bmclaughlin force-pushed the ctl-alt-del-button-for-webmks-consoles branch from 213fbf2 to c4814f6 Compare November 13, 2017 14:58

skateman approved these changes Nov 20, 2017

View reviewed changes

mzazrivec self-assigned this Nov 20, 2017

mzazrivec added this to the Sprint 74 Ending Nov 27, 2017 milestone Nov 20, 2017

mzazrivec merged commit eb1c41a into ManageIQ:master Nov 20, 2017

mzazrivec added consoles gaprindashvili/yes and removed compute/infrastructure labels Nov 20, 2017

mzazrivec added the fine/yes label Nov 20, 2017

This was referenced Nov 20, 2017

Fixed harmless missing CSS error in WebMKS remote consoles #2767

Merged

Adjust the remote console views/controllers for WebMKS proxying #2768

Merged

bmclaughlin deleted the ctl-alt-del-button-for-webmks-consoles branch November 20, 2017 16:01

simaishi added gaprindashvili/backported and removed gaprindashvili/yes labels Nov 20, 2017

simaishi added fine/backported and removed fine/yes labels Nov 28, 2017

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Use the unified console style for WebMKS consoles #2662

Use the unified console style for WebMKS consoles #2662

bmclaughlin commented Nov 7, 2017

bmclaughlin commented Nov 7, 2017

mzazrivec commented Nov 9, 2017

skateman left a comment

skateman Nov 9, 2017

skateman Nov 9, 2017

skateman Nov 9, 2017

skateman Nov 9, 2017

skateman Nov 9, 2017

skateman Nov 9, 2017

skateman Nov 9, 2017

skateman Nov 9, 2017

skateman Nov 9, 2017

skateman Nov 9, 2017

skateman Nov 9, 2017

bmclaughlin Nov 9, 2017

skateman Nov 9, 2017

miq-bot commented Nov 10, 2017

skateman Nov 10, 2017 •

edited

Loading

miq-bot commented Nov 13, 2017

skateman commented Nov 13, 2017

skateman left a comment

simaishi commented Nov 20, 2017

simaishi commented Nov 28, 2017

Use the unified console style for WebMKS consoles #2662

Use the unified console style for WebMKS consoles #2662

Conversation

bmclaughlin commented Nov 7, 2017

bmclaughlin commented Nov 7, 2017

mzazrivec commented Nov 9, 2017

skateman 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

Choose a reason for hiding this comment

miq-bot commented Nov 10, 2017

skateman Nov 10, 2017 • edited Loading

Choose a reason for hiding this comment

miq-bot commented Nov 13, 2017

skateman commented Nov 13, 2017

skateman left a comment

Choose a reason for hiding this comment

simaishi commented Nov 20, 2017

simaishi commented Nov 28, 2017

skateman Nov 10, 2017 •

edited

Loading