Follow-on updates from jennyhaines #2400

sg00dwin · 2017-10-30T20:09:49Z

Requested changes
#2362 (comment) and #2182 (comment)

spadgett · 2017-10-30T20:38:49Z

app/views/directives/key-value-editor.html

@@ -4,14 +4,10 @@
    ng-if="showHeader"
    class="key-value-editor-entry key-value-editor-entry-header">
    <div class="form-group key-value-editor-header key-header">


Do we still need form-group?

well, it's setting the margin-bottom but that could be set using the specific class *-header

If so we'd need to do the same for the KVE ones above to be consistent

spadgett · 2017-10-30T20:38:58Z

app/views/directives/key-value-editor.html

-      <div class="input-group">
-        <span class="help-block">{{keyPlaceholder}}</span>
-      </div>
+      {{keyPlaceholder}}
    </div>
    <div class="form-group key-value-editor-header value-header">


Do we still need form-group?

spadgett · 2017-10-30T20:39:46Z

app/styles/_kve.less

@@ -192,6 +192,12 @@
  margin-bottom: 0;
  padding-right: 5px;
  width: 50%;
+  &.prefix-header {
+    display: none;


If the header is totally hidden, I'm not sure how you know what that field does on mobile.

well when empty it does display placeholder="Add prefix" :)
I'll append it to the single header and show at mobile.

sg00dwin · 2017-10-30T21:25:50Z

@spadgett updated

spadgett · 2017-10-31T13:45:58Z

Hey @sg00dwin I'd suggest keeping the current layout for desktop, but stacking the labels above at mobile. Something like

Config Map / Secret
[my-config-map]

Prefix
_____________

Config Map / Secret
[my-secret]

Prefix
MY_PREFIX_

sg00dwin · 2017-10-31T15:06:35Z

@spadgett Is this the structure you suggest?

spadgett · 2017-10-31T15:13:45Z

Yeah, that's what I was thinking. Seeing it, I'm good with either approach, though.

@beanh66 Opinion?

beanh66 · 2017-10-31T15:27:29Z

This approach LGTM @sg00dwin @spadgett

sg00dwin · 2017-10-31T17:55:59Z

@spadgett PTAL

spadgett · 2017-10-31T20:45:59Z

spadgett · 2017-10-31T20:47:20Z

Nevermind on the prefix width: I see it's to give room for the view details link

spadgett · 2017-10-31T21:00:42Z

app/styles/_tooltip.less

@@ -2,6 +2,10 @@
 // Tooltips
 // --------------------------------------------------

+.tooltip-default-icon {
+  font-size: @font-size-base - 1;
+  vertical-align: top;


I like creating a common style, but currently most other tooltip icons aren't top-aligned. For instance,

Yeah, thought about that. Maybe add a todo to sweep through and common class.

spadgett · 2017-10-31T21:03:09Z

app/views/directives/edit-environment-from.html

        Prefix
-        <small class="pficon pficon-help"
+        <small class="pficon pficon-help tooltip-default-icon"


We should add a style to switch the cursor to the help icon on hover like other tooltips.

spadgett · 2017-10-31T21:05:21Z

app/views/directives/edit-environment-from.html

@@ -62,7 +64,16 @@
        </div>
      </div>

-      <div class="form-group environment-from-input prefix">
+      <div class="environment-from-input prefix">
+        <div


Ideally this is a <label for="..."> for accessibility, but that's a bit tricky since you have to generate IDs. I'm OK leaving that for later.

spadgett · 2017-10-31T21:10:40Z

app/views/directives/edit-environment-variables.html

@@ -1,7 +1,7 @@
 <form ng-if="$ctrl.apiObject" name="$ctrl.form" class="mar-bottom-xl">
  <confirm-on-exit ng-if="$ctrl.canIUpdate && !$ctrl.ngReadonly" dirty="$ctrl.form.$dirty"></confirm-on-exit>
  <div ng-repeat="container in $ctrl.containers">
-    <h3>Container {{container.name}}</h3>
+    <h2>Container {{container.name}}</h2>


I know this change was suggested by @jennyhaines, but I'm a little concerned that we're not making it on other browse pages / other tabs. All of the other headers are h3 on the browse pages.

We might hold off on this and look at it in the large context of all the pages later. Any objections @jennyhaines ?

If so, we'd probably keep the "Environment From" header an h4 (without the section-label class) keep the hierarchy.

openshift#2362 (comment)

sg00dwin · 2017-11-01T13:01:36Z

@spadgett updated with changes PTAL

spadgett

Thanks @sg00dwin

spadgett · 2017-11-01T13:11:04Z

/lgtm

spadgett · 2017-11-01T13:11:13Z

/kind bug

openshift-merge-robot · 2017-11-01T13:49:42Z

Automatic merge from submit-queue.

jennyhaines · 2017-11-01T14:10:31Z

@spadgett @sg00dwin - understandable about the text size change. I think that works for now so the text styles map across the other tab content.

openshift-ci-robot added the size/M Denotes a PR that changes 30-99 lines, ignoring generated files. label Oct 30, 2017

sg00dwin requested a review from spadgett October 30, 2017 20:17

spadgett reviewed Oct 30, 2017

View reviewed changes

sg00dwin force-pushed the env-from-cleanup branch from b577249 to 8806409 Compare October 30, 2017 21:22

openshift-ci-robot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. and removed size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels Oct 30, 2017

sg00dwin force-pushed the env-from-cleanup branch from 8806409 to 14cf0ec Compare October 31, 2017 17:52

spadgett requested changes Oct 31, 2017

View reviewed changes

sg00dwin force-pushed the env-from-cleanup branch from 14cf0ec to cb8cef3 Compare November 1, 2017 12:54

Follow-on updates from @jennyhaines

9b85d2f

openshift#2362 (comment)

sg00dwin force-pushed the env-from-cleanup branch from cb8cef3 to 9b85d2f Compare November 1, 2017 13:01

spadgett approved these changes Nov 1, 2017

View reviewed changes

openshift-ci-robot assigned spadgett Nov 1, 2017

openshift-ci-robot added the lgtm Indicates that a PR is ready to be merged. label Nov 1, 2017

openshift-ci-robot added the kind/bug Categorizes issue or PR as related to a bug. label Nov 1, 2017

openshift-merge-robot merged commit 655e2da into openshift:master Nov 1, 2017

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Follow-on updates from jennyhaines #2400

Follow-on updates from jennyhaines #2400

sg00dwin commented Oct 30, 2017 •

edited

Loading

spadgett Oct 30, 2017

sg00dwin Oct 30, 2017

spadgett Oct 30, 2017

spadgett Oct 30, 2017

sg00dwin Oct 30, 2017

sg00dwin commented Oct 30, 2017

spadgett commented Oct 31, 2017

sg00dwin commented Oct 31, 2017

spadgett commented Oct 31, 2017

beanh66 commented Oct 31, 2017

sg00dwin commented Oct 31, 2017

spadgett commented Oct 31, 2017

spadgett commented Oct 31, 2017

spadgett Oct 31, 2017

sg00dwin Nov 1, 2017

spadgett Oct 31, 2017

spadgett Oct 31, 2017

spadgett Oct 31, 2017

sg00dwin commented Nov 1, 2017 •

edited

Loading

spadgett left a comment

spadgett commented Nov 1, 2017

spadgett commented Nov 1, 2017

openshift-merge-robot commented Nov 1, 2017

jennyhaines commented Nov 1, 2017

Follow-on updates from jennyhaines #2400

Follow-on updates from jennyhaines #2400

Conversation

sg00dwin commented Oct 30, 2017 • edited Loading

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

sg00dwin commented Oct 30, 2017

spadgett commented Oct 31, 2017

sg00dwin commented Oct 31, 2017

spadgett commented Oct 31, 2017

beanh66 commented Oct 31, 2017

sg00dwin commented Oct 31, 2017

spadgett commented Oct 31, 2017

spadgett commented Oct 31, 2017

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

sg00dwin commented Nov 1, 2017 • edited Loading

spadgett left a comment

Choose a reason for hiding this comment

spadgett commented Nov 1, 2017

spadgett commented Nov 1, 2017

openshift-merge-robot commented Nov 1, 2017

jennyhaines commented Nov 1, 2017

sg00dwin commented Oct 30, 2017 •

edited

Loading

sg00dwin commented Nov 1, 2017 •

edited

Loading