Bug 1870471: Fix Tech and Dev preview badge styles #6250

rottencandy · 2020-08-06T15:13:20Z

Fixes:
https://issues.redhat.com/browse/ODC-4440

Analysis / Root cause:
Tech and dev preview badges aren't shown correctly.

Solution Description:
Fix and align badge styles.

Screen shots / Gifs for design review:

Before:

After:

Unit test coverage report:
Unchanged.

Browser conformance:

Chrome
Firefox
Safari
Edge

rottencandy · 2020-08-06T15:15:48Z

/kind bug

andrewballantyne · 2020-08-06T15:25:40Z

frontend/packages/console-shared/src/components/badges/Badge.scss

@@ -5,7 +5,7 @@
  }
 }

-.tech-preview {
+.odc-preview-badge {


Suggested change

.odc-preview-badge {

.osc-preview-badge {

Not part of the DevConsole package, so can't have 'odc' (aka Openshift Developer Console) prefix.

andrewballantyne · 2020-08-06T15:26:02Z

frontend/packages/console-shared/src/components/badges/DevPreviewBadge.tsx

-  <Label style={{ backgroundColor: '#D93F00' }}>Dev Preview</Label>
+  <Label
+    className="odc-preview-badge"
+    style={{ backgroundColor: '#D93F00', borderRadius: 'var(--pf-global--BorderRadius--sm)' }}


Include these styles in the css class

Those styles when added in the class seem to be getting overriden by patternfly.

How about using !important for these styles in the scss?

Thanks! Using !important worked.

andrewballantyne · 2020-08-06T15:26:18Z

frontend/packages/console-shared/src/components/badges/DevPreviewBadge.tsx

@@ -1,8 +1,14 @@
 import * as React from 'react';
 import { Label } from '@patternfly/react-core';
+import './Badge.scss';


Odd - how did this work before?

It would directly use the tech-preview class, not sure if it would work without the import.

vikram-raj · 2020-08-06T15:27:19Z

frontend/packages/console-shared/src/components/badges/Badge.scss

@@ -5,7 +5,7 @@
  }
 }

-.tech-preview {
+.odc-preview-badge {


Suggested change

.odc-preview-badge {

.ocs-preview-badge {

This is not updated. I can still see the preview-badge class not prefixed with ocs. https://github.com/openshift/console/pull/6250/files#diff-0c1edabb529e4a788e55f04adb9c44a3

Had gotten confused with the similar review above, fixed now.

Ah that was my bad... Vikram had the right order. openshift console shared => ocs

rohitkrai03

/lgtm

rottencandy · 2020-08-18T05:52:33Z

/assign @andrewballantyne

rottencandy · 2020-08-18T05:52:40Z

/retest

rottencandy · 2020-08-18T07:52:59Z

/retest

rottencandy · 2020-08-18T09:25:57Z

/retest

rottencandy · 2020-08-18T11:41:53Z

/retest

openshift-ci-robot · 2020-08-20T07:45:40Z

@rottencandy: This pull request references Bugzilla bug 1870471, which is valid. The bug has been moved to the POST state. The bug has been updated to refer to the pull request using the external bug tracker.

3 validation(s) were run on this bug

bug is open, matching expected state (open)
bug target release (4.6.0) matches configured target release for branch (4.6.0)
bug is in the state NEW, which is one of the valid states (NEW, ASSIGNED, ON_DEV, POST, POST)

In response to this:

Bug 1870471: Fix Tech and Dev preview badge styles

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

andrewballantyne · 2020-08-21T17:42:42Z

frontend/packages/console-shared/src/components/badges/Badge.scss

+  background-color: #d93f00 !important;
+  border-radius: var(--pf-global--BorderRadius--sm) !important;


One of the golden rules of CSS, especially those in an app of this size, is !important should be avoided.

@rottencandy Please read the MDN page on CSS Specificity, it'll help bring forth this point.

@sahil143 Please read it as well.

Suggested change

background-color: #d93f00 !important;

border-radius: var(--pf-global--BorderRadius--sm) !important;

&.pf-c-label {

background-color: #d93f00;

border-radius: var(--pf-global--BorderRadius--sm);

}

You can look in the dev tools for Chrome to see how it's calculated specificity for the current element selected. In this case our .ocs-preview-badge was several down, this helps bring it up to the top.

Thanks @andrewballantyne , I've tried using type selectors to improve specificity. PTAL.

andrewballantyne · 2020-08-21T17:44:04Z

frontend/packages/console-shared/src/components/badges/Badge.scss

@@ -5,7 +5,7 @@
  }
 }

-.tech-preview {
+.odc-preview-badge {


Ah that was my bad... Vikram had the right order. openshift console shared => ocs

andrewballantyne

Looks good with the exception of the !important usages.

andrewballantyne · 2020-08-21T17:45:25Z

/lgtm cancel

andrewballantyne · 2020-08-24T16:49:06Z

frontend/packages/console-shared/src/components/badges/Badge.scss

@@ -5,7 +5,9 @@
  }
 }

-.tech-preview {
+span.ocs-preview-badge {


@rottencandy Why span? What was wrong with the suggested change I gave?

The problem here is you're tying these styles to a semantic rendering, and there is nothing tying these styles to a span element.

Sorry, I read your comment on email and didn't notice that you had suggested a change, so tried coming up with my own solution. Updated now.

andrewballantyne

/lgtm

openshift-ci-robot · 2020-08-24T17:08:57Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: andrewballantyne, rohitkrai03, rottencandy

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

~~frontend/packages/console-shared/OWNERS~~ [andrewballantyne,rohitkrai03]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

openshift-ci-robot · 2020-08-24T20:33:54Z

@rottencandy: All pull requests linked via external trackers have merged:

openshift/console#6250

Bugzilla bug 1870471 has been moved to the MODIFIED state.

In response to this:

Bug 1870471: Fix Tech and Dev preview badge styles

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

openshift-ci-robot requested review from jhadvig and rhamilto August 6, 2020 15:13

rottencandy changed the title ~~fix preview badge styles~~ Fix Tech and Dev preview badge styles Aug 6, 2020

openshift-ci-robot added the kind/bug Categorizes issue or PR as related to a bug. label Aug 6, 2020

andrewballantyne reviewed Aug 6, 2020

View reviewed changes

vikram-raj reviewed Aug 6, 2020

View reviewed changes

rottencandy force-pushed the prevbadge branch 2 times, most recently from 59a0c79 to b3d9187 Compare August 11, 2020 07:02

rottencandy force-pushed the prevbadge branch from b3d9187 to d614800 Compare August 17, 2020 07:52

rohitkrai03 approved these changes Aug 18, 2020

View reviewed changes

openshift-ci-robot assigned rohitkrai03 Aug 18, 2020

openshift-ci-robot added the lgtm Indicates that a PR is ready to be merged. label Aug 18, 2020

openshift-ci-robot assigned andrewballantyne Aug 18, 2020

rottencandy changed the title ~~Fix Tech and Dev preview badge styles~~ Bug 1870471: Fix Tech and Dev preview badge styles Aug 20, 2020

openshift-ci-robot added bugzilla/severity-low Referenced Bugzilla bug's severity is low for the branch this PR is targeting. bugzilla/valid-bug Indicates that a referenced Bugzilla bug is valid for the branch this PR is targeting. labels Aug 20, 2020

andrewballantyne reviewed Aug 21, 2020

View reviewed changes

andrewballantyne suggested changes Aug 21, 2020

View reviewed changes

openshift-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Aug 21, 2020

openshift-ci-robot removed the lgtm Indicates that a PR is ready to be merged. label Aug 21, 2020

rottencandy force-pushed the prevbadge branch from d614800 to e2bf8d1 Compare August 24, 2020 07:32

andrewballantyne reviewed Aug 24, 2020

View reviewed changes

fix preview badge styles

950bf40

rottencandy force-pushed the prevbadge branch from e2bf8d1 to 950bf40 Compare August 24, 2020 16:57

andrewballantyne approved these changes Aug 24, 2020

View reviewed changes

openshift-ci-robot added the lgtm Indicates that a PR is ready to be merged. label Aug 24, 2020

openshift-merge-robot merged commit 490b381 into openshift:master Aug 24, 2020

spadgett added this to the v4.6 milestone Aug 26, 2020

rottencandy deleted the prevbadge branch September 8, 2020 08:38

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Bug 1870471: Fix Tech and Dev preview badge styles #6250

Bug 1870471: Fix Tech and Dev preview badge styles #6250

rottencandy commented Aug 6, 2020 •

edited

rottencandy commented Aug 6, 2020

andrewballantyne Aug 6, 2020

rottencandy Aug 6, 2020

andrewballantyne Aug 6, 2020

rottencandy Aug 6, 2020

sahil143 Aug 10, 2020

rottencandy Aug 17, 2020 •

edited

andrewballantyne Aug 6, 2020

rottencandy Aug 6, 2020 •

edited

vikram-raj Aug 6, 2020

rottencandy Aug 6, 2020

sahil143 Aug 10, 2020

rottencandy Aug 11, 2020

andrewballantyne Aug 21, 2020

rohitkrai03 left a comment

rottencandy commented Aug 18, 2020

rottencandy commented Aug 18, 2020

rottencandy commented Aug 18, 2020

rottencandy commented Aug 18, 2020

rottencandy commented Aug 18, 2020

openshift-ci-robot commented Aug 20, 2020

andrewballantyne Aug 21, 2020 •

edited

rottencandy Aug 24, 2020

andrewballantyne Aug 21, 2020

andrewballantyne left a comment

andrewballantyne commented Aug 21, 2020

andrewballantyne Aug 24, 2020

rottencandy Aug 24, 2020

andrewballantyne left a comment

openshift-ci-robot commented Aug 24, 2020

openshift-ci-robot commented Aug 24, 2020

		background-color: #d93f00 !important;
		border-radius: var(--pf-global--BorderRadius--sm) !important;

-  background-color: #d93f00 !important;
-  border-radius: var(--pf-global--BorderRadius--sm) !important;
+  &.pf-c-label {
+    background-color: #d93f00;
+    border-radius: var(--pf-global--BorderRadius--sm);
+  }

Bug 1870471: Fix Tech and Dev preview badge styles #6250

Bug 1870471: Fix Tech and Dev preview badge styles #6250

Conversation

rottencandy commented Aug 6, 2020 • edited

rottencandy commented Aug 6, 2020

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

rottencandy Aug 17, 2020 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rottencandy Aug 6, 2020 • edited

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

rohitkrai03 left a comment

Choose a reason for hiding this comment

rottencandy commented Aug 18, 2020

rottencandy commented Aug 18, 2020

rottencandy commented Aug 18, 2020

rottencandy commented Aug 18, 2020

rottencandy commented Aug 18, 2020

openshift-ci-robot commented Aug 20, 2020

andrewballantyne Aug 21, 2020 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

andrewballantyne left a comment

Choose a reason for hiding this comment

andrewballantyne commented Aug 21, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

andrewballantyne left a comment

Choose a reason for hiding this comment

openshift-ci-robot commented Aug 24, 2020

openshift-ci-robot commented Aug 24, 2020

rottencandy commented Aug 6, 2020 •

edited

rottencandy Aug 17, 2020 •

edited

rottencandy Aug 6, 2020 •

edited

andrewballantyne Aug 21, 2020 •

edited