From 717621698d7b883aa05e36031c92b96c103c0f40 Mon Sep 17 00:00:00 2001
From: John Cowen <johncowen@users.noreply.github.com>
Date: Fri, 18 Feb 2022 17:16:03 +0000
Subject: [PATCH] ui: Replace CollapsibleNotices with more a11y focussed
 Disclosure component (#12305)

* Delete collapsible notices component and related helper

* Add relative t action/helper to our Route component

* Replace single use CollapsibleNotices with multi-use Disclosure
---
 .changelog/12305.txt                          |   3 +
 .../components/collapsible-notices/README.mdx |  66 -------
 .../components/collapsible-notices/index.hbs  |  14 --
 .../components/collapsible-notices/index.js   |   3 -
 .../components/collapsible-notices/index.scss |  31 ----
 .../consul-ui/app/components/route/index.hbs  |   1 +
 .../consul-ui/app/components/route/index.js   |  17 +-
 .../components/topology-metrics/layout.scss   |   8 +
 .../topology-metrics/notice/index.hbs         |  31 ----
 .../app/components/topology-metrics/skin.scss |  13 ++
 .../app/helpers/collapsible-notices.js        |   9 -
 .../consul-ui/app/styles/components.scss      |   1 -
 .../templates/dc/services/show/topology.hbs   | 163 +++++++++++-------
 .../translations/components/consul/en-us.yaml |  28 ---
 .../consul-ui/translations/routes/en-us.yaml  |  43 +++++
 15 files changed, 187 insertions(+), 244 deletions(-)
 create mode 100644 .changelog/12305.txt
 delete mode 100644 ui/packages/consul-ui/app/components/collapsible-notices/README.mdx
 delete mode 100644 ui/packages/consul-ui/app/components/collapsible-notices/index.hbs
 delete mode 100644 ui/packages/consul-ui/app/components/collapsible-notices/index.js
 delete mode 100644 ui/packages/consul-ui/app/components/collapsible-notices/index.scss
 delete mode 100644 ui/packages/consul-ui/app/components/topology-metrics/notice/index.hbs
 delete mode 100644 ui/packages/consul-ui/app/helpers/collapsible-notices.js

diff --git a/.changelog/12305.txt b/.changelog/12305.txt
new file mode 100644
index 0000000000..a81fbd9edc
--- /dev/null
+++ b/.changelog/12305.txt
@@ -0,0 +1,3 @@
+```release-note:enhancement
+ui: Improve usability of Topology warning/information panels
+```
diff --git a/ui/packages/consul-ui/app/components/collapsible-notices/README.mdx b/ui/packages/consul-ui/app/components/collapsible-notices/README.mdx
deleted file mode 100644
index c6cc7f06e9..0000000000
--- a/ui/packages/consul-ui/app/components/collapsible-notices/README.mdx
+++ /dev/null
@@ -1,66 +0,0 @@
-# CollapsibleNotices
-
-Used as a wrapper to collapse the details of `<Notices/>`.
-
-```hbs preview-template
-<CollapsibleNotices>
-  <Notice
-    @type="error"
-    role="alert"
-  as |notice|>
-    <notice.Header>
-      <h3>Header</h3>
-    </notice.Header>
-    <notice.Body>
-      <p>
-        Body
-      </p>
-    </notice.Body>
-  </Notice>
-  <Notice
-    @type="info"
-  as |notice|>
-    <notice.Header>
-      <h3>Header</h3>
-    </notice.Header>
-    <notice.Body>
-      <p>
-        Body
-      </p>
-    </notice.Body>
-    <notice.Footer>
-      <p>
-        Footer
-      </p>
-    </notice.Footer>
-  </Notice>
-  <Notice
-    @type="warning"
-  as |notice|>
-    <notice.Header>
-      <h3>Header</h3>
-    </notice.Header>
-    <notice.Body>
-      <p>
-        Body
-      </p>
-    </notice.Body>
-    <notice.Footer>
-      <p>
-        Footer
-      </p>
-    </notice.Footer>
-  </Notice>
-</CollapsibleNotices>
-
-```
-
-## Arguments
-
-No arguments required. Wrap this component around the needed notices.
-
-## See
-
-- [Template Source Code](./index.hbs)
-
----
\ No newline at end of file
diff --git a/ui/packages/consul-ui/app/components/collapsible-notices/index.hbs b/ui/packages/consul-ui/app/components/collapsible-notices/index.hbs
deleted file mode 100644
index 6d643d4ff3..0000000000
--- a/ui/packages/consul-ui/app/components/collapsible-notices/index.hbs
+++ /dev/null
@@ -1,14 +0,0 @@
-{{#if @collapsible}}
-  <div class="collapsible-notices {{if this.collapsed 'collapsed'}}">
-    <div class="notices">
-      {{yield}}
-    </div>
-    {{#if this.collapsed}}
-    <button type="button" class="expand" {{on 'click' (set this 'collapsed' false)}}>{{t "components.app.collapsible-notices.expand"}}</button>
-    {{else}}
-    <button type="button" class="collapse" {{on 'click' (set this 'collapsed' true)}}>{{t "components.app.collapsible-notices.collapse"}}</button>
-    {{/if}}
-  </div>
-{{else}}
-  {{yield}}
-{{/if}}
diff --git a/ui/packages/consul-ui/app/components/collapsible-notices/index.js b/ui/packages/consul-ui/app/components/collapsible-notices/index.js
deleted file mode 100644
index 8b246f27e2..0000000000
--- a/ui/packages/consul-ui/app/components/collapsible-notices/index.js
+++ /dev/null
@@ -1,3 +0,0 @@
-import Component from '@glimmer/component';
-
-export default class CollapsibleNotices extends Component {}
diff --git a/ui/packages/consul-ui/app/components/collapsible-notices/index.scss b/ui/packages/consul-ui/app/components/collapsible-notices/index.scss
deleted file mode 100644
index 672904fce3..0000000000
--- a/ui/packages/consul-ui/app/components/collapsible-notices/index.scss
+++ /dev/null
@@ -1,31 +0,0 @@
-.collapsible-notices {
-  display: grid;
-  grid-template-columns: auto 168px;
-  grid-template-rows: auto 55px;
-  grid-template-areas:
-    'notices notices'
-    '. toggle-button';
-  &.collapsed p {
-    display: none;
-  }
-  .notices {
-    grid-area: notices;
-    :last-child {
-      margin-bottom: 0;
-    }
-  }
-  button {
-    @extend %button;
-    color: rgb(var(--color-action));
-    float: right;
-    grid-area: toggle-button;
-    margin-top: 1em;
-    margin-bottom: 2em;
-  }
-  button.expand::before {
-    @extend %with-chevron-down-mask, %as-pseudo;
-  }
-  button.collapse::before {
-    @extend %with-chevron-up-mask, %as-pseudo;
-  }
-}
diff --git a/ui/packages/consul-ui/app/components/route/index.hbs b/ui/packages/consul-ui/app/components/route/index.hbs
index 783e62db54..7af35e58f4 100644
--- a/ui/packages/consul-ui/app/components/route/index.hbs
+++ b/ui/packages/consul-ui/app/components/route/index.hbs
@@ -6,6 +6,7 @@
   currentName=this.router.currentRoute.name
 
   refresh=this.refresh
+  t=this.t
 
   Title=(component "route/title")
   Announcer=(component "route/announcer")
diff --git a/ui/packages/consul-ui/app/components/route/index.js b/ui/packages/consul-ui/app/components/route/index.js
index d484560c1c..999ae7fd63 100644
--- a/ui/packages/consul-ui/app/components/route/index.js
+++ b/ui/packages/consul-ui/app/components/route/index.js
@@ -3,18 +3,26 @@ import { inject as service } from '@ember/service';
 import { action } from '@ember/object';
 import { tracked } from '@glimmer/tracking';
 
+const templateRe = /\${([A-Za-z.0-9_-]+)}/g;
 export default class RouteComponent extends Component {
   @service('routlet') routlet;
   @service('router') router;
+  @service('intl') intl;
+  @service('encoder') encoder;
 
   @tracked _model;
 
+  constructor() {
+    super(...arguments);
+    this.intlKey = this.encoder.createRegExpEncoder(templateRe, _ => _);
+  }
+
   get params() {
     return this.routlet.paramsFor(this.args.name);
   }
 
   get model() {
-    if(this._model) {
+    if (this._model) {
       return this._model;
     }
     if (this.args.name) {
@@ -23,6 +31,13 @@ export default class RouteComponent extends Component {
     }
     return undefined;
   }
+  @action
+  t(str, options) {
+    if (str.includes('${')) {
+      str = this.intlKey(str, options);
+    }
+    return this.intl.t(`routes.${this.args.name}.${str}`, options);
+  }
 
   @action
   connect() {
diff --git a/ui/packages/consul-ui/app/components/topology-metrics/layout.scss b/ui/packages/consul-ui/app/components/topology-metrics/layout.scss
index 215efad41a..e62a58884b 100644
--- a/ui/packages/consul-ui/app/components/topology-metrics/layout.scss
+++ b/ui/packages/consul-ui/app/components/topology-metrics/layout.scss
@@ -1,3 +1,11 @@
+.topology-notices {
+  display: flow-root;
+  button {
+    float: right;
+    margin-top: 16px;
+    margin-bottom: 32px;
+  }
+}
 .topology-container {
   display: grid;
   height: 100%;
diff --git a/ui/packages/consul-ui/app/components/topology-metrics/notice/index.hbs b/ui/packages/consul-ui/app/components/topology-metrics/notice/index.hbs
deleted file mode 100644
index db8ac82bbd..0000000000
--- a/ui/packages/consul-ui/app/components/topology-metrics/notice/index.hbs
+++ /dev/null
@@ -1,31 +0,0 @@
-<Notice
-  class="topology-metrics-notice"
-  ...attributes
-  @type={{@type}}
-as |notice|>
-    <notice.Header>
-    <h3>
-      {{t (concat "components.consul.topology-metrics.notice." @for ".header")}}
-    </h3>
-  </notice.Header>
-  <notice.Body>
-    <p>
-     {{t (concat "components.consul.topology-metrics.notice." @for ".body")}}
-    </p>
-  </notice.Body>
-{{#if @action}}
-  <notice.Footer>
-    <p>
-      {{#if @internal}}
-        <Action @href={{href-to (t (concat "components.consul.topology-metrics.notice." @for ".footer.URL"))}}>
-          {{t (concat "components.consul.topology-metrics.notice." @for ".footer.name")}}
-        </Action>
-      {{else}}
-        <Action @href={{@link}} @external={{true}}>
-          {{t (concat "components.consul.topology-metrics.notice." @for ".footer")}}
-        </Action>
-      {{/if}}
-    </p>
-  </notice.Footer>
-{{/if}}
-</Notice>
\ No newline at end of file
diff --git a/ui/packages/consul-ui/app/components/topology-metrics/skin.scss b/ui/packages/consul-ui/app/components/topology-metrics/skin.scss
index 06fe495882..fc819a6300 100644
--- a/ui/packages/consul-ui/app/components/topology-metrics/skin.scss
+++ b/ui/packages/consul-ui/app/components/topology-metrics/skin.scss
@@ -1,3 +1,16 @@
+.topology-notices {
+  button {
+    @extend %button;
+    color: rgb(var(--tone-blue-500));
+  }
+  button::before {
+    @extend %as-pseudo;
+    @extend %with-chevron-down-mask;
+  }
+  button[aria-expanded='true']::before {
+    @extend %with-chevron-up-mask;
+  }
+}
 .topology-container {
   color: rgb(var(--tone-gray-700));
 }
diff --git a/ui/packages/consul-ui/app/helpers/collapsible-notices.js b/ui/packages/consul-ui/app/helpers/collapsible-notices.js
deleted file mode 100644
index e9e8925e2a..0000000000
--- a/ui/packages/consul-ui/app/helpers/collapsible-notices.js
+++ /dev/null
@@ -1,9 +0,0 @@
-import { helper } from '@ember/component/helper';
-
-export function collapsibleNotices(params, hash) {
-  // This filter will only return truthy items
-  const noticesCount = params.filter(Boolean).length;
-  return noticesCount > 2;
-}
-
-export default helper(collapsibleNotices);
diff --git a/ui/packages/consul-ui/app/styles/components.scss b/ui/packages/consul-ui/app/styles/components.scss
index f90d8fe299..d6f33f5333 100644
--- a/ui/packages/consul-ui/app/styles/components.scss
+++ b/ui/packages/consul-ui/app/styles/components.scss
@@ -92,7 +92,6 @@
 @import 'consul-ui/components/consul/auth-method';
 
 @import 'consul-ui/components/role-selector';
-@import 'consul-ui/components/collapsible-notices';
 @import 'consul-ui/components/topology-metrics';
 @import 'consul-ui/components/topology-metrics/card';
 @import 'consul-ui/components/topology-metrics/source-type';
diff --git a/ui/packages/consul-ui/app/templates/dc/services/show/topology.hbs b/ui/packages/consul-ui/app/templates/dc/services/show/topology.hbs
index a8df87e411..6c950a255d 100644
--- a/ui/packages/consul-ui/app/templates/dc/services/show/topology.hbs
+++ b/ui/packages/consul-ui/app/templates/dc/services/show/topology.hbs
@@ -27,66 +27,107 @@ as |route|>
   loader.data
 as |nspace dc items topology|}}
   <div class="tab-section">
-  {{#let (collapsible-notices topology.FilteredByACLs (eq dc.DefaultACLPolicy 'allow') topology.wildcardIntention topology.notDefinedIntention) as |collapsible| }}
-    <CollapsibleNotices @collapsible={{collapsible}}>
-      {{#if topology.FilteredByACLs}}
-        <TopologyMetrics::Notice
-          data-test-notice='filtered-by-acls'
-          @type="info"
-          @for="limited-access"
-          @action={{false}}
-        />
-      {{/if}}
-      {{#if (eq dc.DefaultACLPolicy 'allow')}}
-        <TopologyMetrics::Notice
-          data-test-notice='default-allow'
-          @type="warning"
-          @for="default-allow"
-          @internal={{true}}
-          @action={{true}}
-        />
-      {{/if}}
-      {{#if topology.wildcardIntention}}
-        <TopologyMetrics::Notice
-          data-test-notice='wildcard-intention'
-          @type="warning"
-          @for="wildcard-intention"
-          @internal={{true}}
-          @action={{true}}
-        />
-      {{/if}}
-      {{#if topology.notDefinedIntention}}
-        <TopologyMetrics::Notice
-          data-test-notice='not-defined-intention'
-          @type="warning"
-          @for="not-defined-intention"
-          @link="{{env 'CONSUL_DOCS_URL'}}/connect/registration/service-registration#upstreams"
-          @internal={{false}}
-          @action={{true}}
-        />
-      {{/if}}
-      {{#if (and topology.noDependencies (can 'use acls'))}}
-        <TopologyMetrics::Notice
-          data-test-notice='no-dependencies'
-          @type="info"
-          @for="no-dependencies"
-          @link="{{env 'CONSUL_DOCS_URL'}}/connect/registration/service-registration#upstream-configuration-reference"
-          @internal={{false}}
-          @action={{true}}
-        />
-      {{/if}}
-      {{#if (and topology.noDependencies (not (can 'use acls')))}}
-        <TopologyMetrics::Notice
-          data-test-notice='acls-disabled'
-          @type="warning"
-          @for="acls-disabled"
-          @link="{{env 'CONSUL_DOCS_URL'}}/security/acl/acl-system#configuring-acls"
-          @internal={{false}}
-          @action={{true}}
-        />
-      {{/if}}
-    </CollapsibleNotices>
-  {{/let}}
+
+    <div
+      class="topology-notices"
+    >
+      <Disclosure
+        @expanded={{true}}
+        as |disclosure|
+      >
+        {{! Make a map of key=enabled to tell us which notices are enabled }}
+        {{! The `false` here is so we can easily open all the notices up by changing to `true` }}
+        {{#let
+          (from-entries (array
+            (array 'filtered-by-acls' (or false topology.FilteredByACLs))
+            (array 'default-allow' (or false (eq dc.DefaultACLPolicy 'allow')))
+            (array 'wildcard-intention' (or false topology.wildcardIntention))
+            (array 'not-defined-intention' (or false topology.notDefinedIntention))
+            (array 'no-dependencies' (or false (and topology.noDependencies (can 'use acls'))))
+            (array 'acls-disabled' (or false (and topology.noDependencies (not (can 'use acls')))))
+          ))
+        as |notices|}}
+
+          {{! Make an array only of truthy values to we know how many notices are enabled }}
+          {{#let (without false
+            (values notices)
+          ) as |noticesEnabled|}}
+
+            {{! Render any enabled notices }}
+            {{#each-in notices as |prop enabled|}}
+              {{#if enabled}}
+                <disclosure.Details
+                  @auto={{false}}
+                as |details|>
+                  <Notice
+                    class="topology-metrics-notice"
+                    id={{details.id}}
+                    data-test-notice={{prop}}
+                    @type={{if (contains prop (array 'filtered-by-acls' 'no-dependencies'))
+                      'info'
+                      'warning'
+                    }}
+                  as |notice|>
+                    <notice.Header>
+                      <h3>
+                        {{compute (fn route.t 'notice.${prop}.header'
+                          (hash
+                            prop=prop
+                          )
+                        )}}
+                      </h3>
+                    </notice.Header>
+                    {{#if disclosure.expanded}}
+                      <notice.Body>
+                        <p>
+                          {{compute (fn route.t 'notice.${prop}.body'
+                            (hash
+                              prop=prop
+                            )
+                          )}}
+                        </p>
+                      </notice.Body>
+                    {{/if}}
+                    {{#let
+                      (compute (fn route.t 'notice.${prop}.footer'
+                        (hash
+                          route_intentions=(href-to 'dc.services.show.intentions')
+                          prop=prop
+                          htmlSafe=true
+                        )
+                      ))
+                    as |footer|}}
+                      {{#if (and disclosure.expanded (not-eq prop 'filtered-by-acls'))}}
+                        <notice.Footer>
+                          {{footer}}
+                        </notice.Footer>
+                      {{/if}}
+                    {{/let}}
+                  </Notice>
+                </disclosure.Details>
+              {{/if}}
+            {{/each-in}}
+
+            {{! If more than 2 notices are enabled let the user close them }}
+            {{#if (gt noticesEnabled.length 2)}}
+              <disclosure.Action
+                {{on 'click' disclosure.toggle}}
+              >
+              {{compute (fn route.t 'notices.${expanded}'
+                (hash
+                  expanded=(if disclosure.expanded 'close' 'open')
+                )
+              )}}
+              </disclosure.Action>
+            {{/if}}
+
+          {{/let}}
+        {{/let}}
+      </Disclosure>
+
+    </div>
+
+
     <DataSource
       @src={{uri '/${partition}/${nspace}/${dc}/ui-config'
         (hash
@@ -97,6 +138,7 @@ as |nspace dc items topology|}}
       }}
     as |config|>
     {{#if config.data}}
+
       <TopologyMetrics
         @nspace={{nspace}}
         @dc={{dc}}
@@ -117,6 +159,7 @@ as |nspace dc items topology|}}
         @hasMetricsProvider={{gt config.data.metrics_provider.length 0}}
         @oncreate={{route-action 'createIntention'}}
       />
+
     {{/if}}
     </DataSource>
   </div>
diff --git a/ui/packages/consul-ui/translations/components/consul/en-us.yaml b/ui/packages/consul-ui/translations/components/consul/en-us.yaml
index 03f3cd84cc..48975d53d2 100644
--- a/ui/packages/consul-ui/translations/components/consul/en-us.yaml
+++ b/ui/packages/consul-ui/translations/components/consul/en-us.yaml
@@ -123,34 +123,6 @@ topology-metrics:
     routing-config:
       tooltip: This is not a registered Consul service. It’s a routing configuration that routes traffic to real services in Consul.
       text: Routing configuration
-  notice:
-    limited-access:
-      header: Limited Access
-      body: This service may have dependencies you won’t see because you don’t have access to them.
-    default-allow:
-      header: Intentions are set to default allow
-      body: Your Intention settings are currently set to default allow. This means that this view will show connections to every service in your cluster. We recommend changing your Intention settings to default deny and creating specific Intentions for upstream and downstream services for this view to be useful.
-      footer:
-        name: Edit intentions
-        URL: dc.services.show.intentions
-    not-defined-intention:
-      header: Connections are not explicitly defined
-      body: There appears to be an Intention allowing traffic, but the services are unable to communicate until that connection is enabled by defining an explicit upstream or proxies are set to 'transparent' mode.
-      footer: Read the documentation
-    wildcard-intention:
-      header: Permissive Intention
-      body: One or more of your Intentions are set to allow traffic to and/or from all other services in a namespace. This Topology view will show all of those connections if that remains unchanged. We recommend setting more specific Intentions for upstream and downstream services to make this vizualization more useful.
-      footer:
-        name: Edit intentions
-        URL: dc.services.show.intentions
-    no-dependencies:
-      header: No dependencies
-      body: The service you are viewing currently has no dependencies. You will only see metrics for the current service until dependencies are added.
-      footer: Read the documentation
-    acls-disabled:
-      header: Enable ACLs
-      body: This connect-native service may have dependencies, but Consul isn't aware of them when ACLs are disabled. Enable ACLs to make this view more useful.
-      footer: Read the documentation
   popover:
     l7:
       header: Layer 7 permissions
diff --git a/ui/packages/consul-ui/translations/routes/en-us.yaml b/ui/packages/consul-ui/translations/routes/en-us.yaml
index 47462dc2ae..ffa2a2c94a 100644
--- a/ui/packages/consul-ui/translations/routes/en-us.yaml
+++ b/ui/packages/consul-ui/translations/routes/en-us.yaml
@@ -17,6 +17,49 @@ dc:
             </p>
   services:
     show:
+      topology:
+        notices:
+          open: Expand Banners
+          close: Collapse Banners
+        notice:
+          filtered-by-acls:
+            header: Limited Access
+            body: This service may have dependencies you won’t see because you don’t have access to them.
+          default-allow:
+            header: Intentions are set to default allow
+            body: Your Intention settings are currently set to default allow. This means that this view will show connections to every service in your cluster. We recommend changing your Intention settings to default deny and creating specific Intentions for upstream and downstream services for this view to be useful.
+            footer: |
+              <p>
+                <a href="{route_intentions}">Edit Intentions</a>
+              </p>
+          wildcard-intention:
+            header: Permissive Intention
+            body: One or more of your Intentions are set to allow traffic to and/or from all other services in a namespace. This Topology view will show all of those connections if that remains unchanged. We recommend setting more specific Intentions for upstream and downstream services to make this vizualization more useful.
+            footer: |
+              <p>
+                <a href="{route_intentions}">Edit Intentions</a>
+              </p>
+          not-defined-intention:
+            header: Connections are not explicitly defined
+            body: There appears to be an Intention allowing traffic, but the services are unable to communicate until that connection is enabled by defining an explicit upstream or proxies are set to 'transparent' mode.
+            footer: |
+              <p>
+                <a href="{CONSUL_DOCS_URL}/connect/registration/service-registration#upstreams" target="_blank" rel="noopener noreferrer">Read the documentation</a>
+              </p>
+          no-dependencies:
+            header: No dependencies
+            body: The service you are viewing currently has no dependencies. You will only see metrics for the current service until dependencies are added.
+            footer: |
+              <p>
+                <a href="{CONSUL_DOCS_URL}/connect/registration/service-registration#upstream-configuration-reference" target="_blank" rel="noopener noreferrer">Read the documentation</a>
+              </p>
+          acls-disabled:
+            header: Enable ACLs
+            body: This connect-native service may have dependencies, but Consul isn't aware of them when ACLs are disabled. Enable ACLs to make this view more useful.
+            footer: |
+              <p>
+                <a href="{CONSUL_DOCS_URL}/security/acl/acl-system#configuring-acls" target="_blank" rel="noopener noreferrer">Read the documentation</a>
+              </p>
       upstreams:
         intro: |
           <p>