Skip to content

[WIP] Add ConsoleNotification CRD #1284

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Closed
rhamilto wants to merge 1 commit into from
Closed

[WIP] Add ConsoleNotification CRD #1284

rhamilto wants to merge 1 commit into from

Conversation

@rhamilto
Copy link
Member

@rhamilto rhamilto commented Mar 13, 2019 •
edited
Loading

https://jira.coreos.com/browse/CONSOLE-552

Screen Shot 2019-03-13 at 1 42 58 PM

TODOS:

  • add ability to specify background color
  • add ability to specify text color
  • add ability to specify position (top, bottom, both)

@openshift-ci-robot openshift-ci-robot added the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Mar 13, 2019
@openshift-ci-robot
Copy link
Contributor

openshift-ci-robot commented Mar 13, 2019

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: rhamilto

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

The pull request process is described here

Details Needs approval from an approver in each of these files:

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 openshift-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Mar 13, 2019
@openshift-ci-robot openshift-ci-robot added the size/L Denotes a PR that changes 100-499 lines, ignoring generated files. label Mar 13, 2019
@rhamilto rhamilto force-pushed the console-notification-crd branch from c8f8433 to fc67ea2 Compare March 13, 2019 17:43
benjaminapetersen
benjaminapetersen reviewed Mar 13, 2019
View reviewed changes
benjaminapetersen
benjaminapetersen reviewed Mar 13, 2019
View reviewed changes
extensions/console-notification.crd.yaml Outdated
Copy link
Contributor

@benjaminapetersen benjaminapetersen Mar 13, 2019

Choose a reason for hiding this comment

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

Any desire to validate the href?

Copy link
Member Author

@rhamilto rhamilto Mar 13, 2019 •
edited
Loading

Choose a reason for hiding this comment

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

We could, but both relative and absolute links will work, so I'm not sure it's worth it?

Copy link
Member

@jhadvig jhadvig Mar 18, 2019

Choose a reason for hiding this comment

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

Any intention to distinguish between internal and external links ?

benjaminapetersen
benjaminapetersen reviewed Mar 13, 2019
View reviewed changes
extensions/console-notification.crd.yaml Outdated
Copy link
Contributor

@benjaminapetersen benjaminapetersen Mar 13, 2019

Choose a reason for hiding this comment

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

Possibly a target option as well? If the notification is a link out to another tool, do we want the option for a new tab so as not to lose console context?

Copy link
Member Author

@rhamilto rhamilto Mar 13, 2019

Choose a reason for hiding this comment

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

I personally don't think the use of target="_blank" is good (see https://www.nngroup.com/articles/the-top-ten-web-design-mistakes-of-1999/), but I will add the feature if others think it worthwhile.

Copy link
Member Author

@rhamilto rhamilto Mar 19, 2019

Choose a reason for hiding this comment

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

@alimobrem, @robszumski, @openshift/team-ux-review, please chime in here.

To put a finer point on why I disagree with opening links in a new window:

  1. it breaks the back button (see #1)
  2. it's a user-hostile action. IMO, the user should decide where links open. (see #2)

That said, we open links in a new window elsewhere in the console, and providing the option to open links in a new window seems reasonable. That said, once we've added the option, it's much harder to take it away.

Copy link
Member

@spadgett spadgett Mar 19, 2019

Choose a reason for hiding this comment

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

Currently, we open all external links in a new window. I'd think we'd want these links to always open in a new window (and skip the option). I realize there's debate on this, but it would be consistent with current behavior :)

Copy link
Member Author

@rhamilto rhamilto Mar 19, 2019

Choose a reason for hiding this comment

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

Worth noting, we are not currently requiring these links be absolute. Both / and http://www.example.com are valid, so always opening in a new window isn't coupled to whether or not the link is external.

Copy link
Member

@spadgett spadgett Mar 19, 2019

Choose a reason for hiding this comment

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

We probably need some validation. At the very least, we can't accept relative URLs since the URL for every page is different (and the banner is visible on every page).

Copy link

@ncameronbritt ncameronbritt Mar 19, 2019

Choose a reason for hiding this comment

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

I lost the link to the BZ when I closed bluejueans. Is having a link in that message even a customer requirement at this point?

spadgett
spadgett reviewed Mar 13, 2019
View reviewed changes
benjaminapetersen
benjaminapetersen reviewed Mar 13, 2019
View reviewed changes
extensions/console-notification.crd.yaml Outdated
Copy link
Contributor

@benjaminapetersen benjaminapetersen Mar 13, 2019

Choose a reason for hiding this comment

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

Title for accessibility? I'm sure many will be tempted to click here.

@rhamilto rhamilto force-pushed the console-notification-crd branch from fc67ea2 to ad8df53 Compare March 13, 2019 20:28
@rhamilto
Copy link
Member Author

rhamilto commented Mar 13, 2019

@alimobrem, @spadgett mentioned there may be additional requirements for governmental compliance (e.g., specific color of the notification). Are you aware of any such requirements?

spadgett
spadgett reviewed Mar 13, 2019
View reviewed changes
@rhamilto rhamilto force-pushed the console-notification-crd branch 2 times, most recently from 2d54046 to 20fafb7 Compare March 14, 2019 13:34
ohadlevy
ohadlevy reviewed Mar 14, 2019
View reviewed changes
@rhamilto rhamilto force-pushed the console-notification-crd branch 3 times, most recently from 8a27cac to a518c8f Compare March 14, 2019 15:24
jhadvig
jhadvig reviewed Mar 18, 2019
View reviewed changes
extensions/console-notification.crd.yaml Outdated
Copy link
Member

@jhadvig jhadvig Mar 18, 2019

Choose a reason for hiding this comment

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

Would add the message and maybe the link as well, so user can see it though the CLI

Copy link
Member Author

@rhamilto rhamilto Mar 19, 2019

Choose a reason for hiding this comment

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

Good thought. One possible down side is long messages make a mess of the table layout. Thoughts?

Screen Shot 2019-03-19 at 11 34 47 AM

Copy link
Contributor

@benjaminapetersen benjaminapetersen Mar 19, 2019

Choose a reason for hiding this comment

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

@juanvallejo halp :) kidding.

Copy link
Member

@jhadvig jhadvig Mar 19, 2019

Choose a reason for hiding this comment

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

@rhamilto yeah I see your point but still think that it's better to see msgs in the cli then having almost empty output.

extensions/console-notification.crd.yaml Outdated
Copy link
Member

@jhadvig jhadvig Mar 18, 2019

Choose a reason for hiding this comment

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

Any intention to distinguish between internal and external links ?

@rhamilto rhamilto force-pushed the console-notification-crd branch from a518c8f to 057a101 Compare March 19, 2019 15:16
@openshift-ci-robot
Copy link
Contributor

openshift-ci-robot commented Mar 19, 2019

@rhamilto: The following tests failed, say /retest to rerun them all:

Test name Commit Details Rerun command
ci/prow/console-e2e 2d540460573064971def2b9eafaab7cc4197be87 link /test console-e2e
ci/prow/e2e-aws-console-olm 057a101 link /test e2e-aws-console-olm
ci/prow/e2e-aws 057a101 link /test e2e-aws
ci/prow/e2e-aws-console 057a101 link /test e2e-aws-console

Full PR test history. Your PR dashboard. Please help us cut down on flakes by linking to an open issue when you hit one in your PR.

Details

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. I understand the commands that are listed here.

This was referenced Mar 22, 2019
Closed
Closed
@rhamilto
Copy link
Member Author

rhamilto commented Mar 29, 2019

Closing in favor of #1325

@rhamilto rhamilto closed this Mar 29, 2019
@rhamilto rhamilto deleted the console-notification-crd branch April 8, 2019 12:49
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@spadgett spadgett spadgett left review comments

@jhadvig jhadvig jhadvig left review comments

+3 more reviewers

@ohadlevy ohadlevy ohadlevy left review comments

@benjaminapetersen benjaminapetersen benjaminapetersen left review comments

@ncameronbritt ncameronbritt ncameronbritt left review comments

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

approved Indicates a PR has been approved by an approver from all required OWNERS files. do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. size/L Denotes a PR that changes 100-499 lines, ignoring generated files.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

7 participants

@rhamilto @openshift-ci-robot @ohadlevy @benjaminapetersen @spadgett @jhadvig @ncameronbritt