feat: New gcds-notice component #627

ethanWallace · 2024-08-27T19:06:40Z

Summary | Résumé

The notice is a prominent message to draw attention to important information or change.

<gcds-notice
  type=""
  notice-title=""
>
  <slot></slot>
</gcds-notice>

Requires tokens from cds-snc/gcds-tokens#330. (Merged and installed)

melaniebmn

Added some comments with changes.

packages/web/src/index.html

packages/web/src/components/gcds-notice/gcds-notice.tsx

packages/web/src/components/gcds-notice/gcds-notice.css

daine · 2024-09-04T23:52:11Z

packages/web/src/components/gcds-notice/readme.md

@@ -0,0 +1,35 @@
+# gcds-alert


Interesting, do you know why this says gcds-alert?

I copied over the alert files as a base and just missed replacing this one thing 😄

daine · 2024-09-05T00:01:02Z

packages/web/src/components/gcds-notice/gcds-notice.tsx

+   */
+
+  /**
+   * Defines notice role.


For consistency, could we change this to

Suggested change

* Defines notice role.

* Set notice role.

I've mostly seen us use "Set" here but open to other options

daine · 2024-09-05T00:01:16Z

packages/web/src/components/gcds-notice/gcds-notice.tsx

+  }
+
+  /**
+   * Defines the notice title.


Same as above

Suggested change

* Defines the notice title.

* Set the notice title.

daine · 2024-09-05T00:02:36Z

packages/web/src/components/gcds-notice/gcds-notice.tsx

+                  <strong class="notice__type">
+                    {i18n[lang]['type'][type]}
+                  </strong>
+                  {/* Hidden colon to provide pause between caption and heading text for AT */}


Does AT mean accessibility tree?

Suggested change

{/* Hidden colon to provide pause between caption and heading text for AT */}

{/* Hidden colon to provide pause between caption and heading text for accessibility tree */}

daine · 2024-09-05T00:04:05Z

packages/web/src/components/gcds-notice/test/gcds-notice.spec.tsx

+import { newSpecPage } from '@stencil/core/testing';
+import { GcdsNotice } from '../gcds-notice';
+
+describe('gcds-alert', () => {


Maybe a typo? 😄

Suggested change

describe('gcds-alert', () => {

describe('gcds-notice', () => {

melaniebmn

Thank you for making all the changes! Looks good to me, I'll hold off on approving until we hear back about the DTO alignment.

duboisp

I quickly went through this PR and I would like to propose the following 2 additions:

Add a property to configure the heading level for the notice. It is not always at level 2.
Add a property to configure the text representing the type of alert. Sometime a web author might want to use alternate words to represent the type of notice for better integration into existing web content.

Thought?

duboisp · 2024-09-16T19:41:00Z

packages/web/src/components/gcds-notice/gcds-notice.tsx

+      <Host>
+        {this.validateRequiredProps() && (
+          <section class={`gcds-notice notice--type-${type}`}>
+            <gcds-heading tag="h2" marginTop="0" class="notice__heading">


The use of [tag=h2] is a good default but there can be some situation where a web author would like to set if for a different heading level.

duboisp · 2024-09-16T20:02:26Z

packages/web/src/components/gcds-notice/gcds-notice.tsx

+          <section class={`gcds-notice notice--type-${type}`}>
+            <gcds-heading tag="h2" marginTop="0" class="notice__heading">
+              <strong class="notice__type">
+                {i18n[lang]['type'][type]}


What about if the web author want to use alternative words which do also provide a sense of the type of notice?

Take for example an impromptu office closure, IMHO the following notice wont be desirable as there is not necessary means an immediate danger for the public:

Danger: Office closure
The office X was closed for the day due to Y situation.

May be they it will be preferable to have something more contextualized which should be rendered as a danger notice type like:

Unplanned: Office closure
The office X was closed for the day due to Y situation.

I would like to propose to add a new property to allow the web author to customization the text conveying the importance level of the notice. If not provided, it can fallback on this default.

Thought?

Thank you for the input! We have made the decision to change the design of the notice component to back to the design already available from canada.ca (with the icon). We just haven't had a chance to update this PR yet with those design changes

daine

I have one small suggestion for the code to display the icons. I know we don't have the tokens yet so I'll review again once we have that in place. At the moment I don't see the lines on the icon, I'm guessing we need the tokens for that

daine · 2024-10-02T02:21:21Z

packages/web/src/components/gcds-notice/gcds-notice.tsx

+              name={
+                type === 'danger'
+                  ? 'exclamation-circle'
+                  : type === 'info'
+                    ? 'info-circle'
+                    : type === 'success'
+                      ? 'check-circle'
+                      : type === 'warning'
+                        ? 'exclamation-triangle'
+                        : null
+              }


Could change this to a lookup instead of multiple if-else to optimize it?
Something like:

let iconType = null; let iconTypes = { 'danger': 'exclamation-circle', 'info': 'info-circle', 'success': 'check-circle', 'warning': 'exclamation-triangle' }

and then:

name={iconTypes[type]}

ethanWallace · 2024-10-02T12:44:48Z

I have one small suggestion for the code to display the icons. I know we don't have the tokens yet so I'll review again once we have that in place. At the moment I don't see the lines on the icon, I'm guessing we need the tokens for that

@daine Would you be able to test with the tokens package linked? It helps identify if a mistake snuck through in the token package before it is published

melaniebmn

LGTM

ethanWallace added 4 commits August 26, 2024 15:05

First commit: new gcds-notice component

65e945b

gcds-notice stories

665504f

gcds-notice spec and e2e tests

8ba7911

fix CSS + format index.html

662bb8a

ethanWallace mentioned this pull request Aug 27, 2024

feat: Add notice component tokens + new colours cds-snc/gcds-tokens#313

Merged

ethanWallace requested a review from melaniebmn August 29, 2024 13:35

ethanWallace marked this pull request as ready for review August 29, 2024 13:35

ethanWallace added 2 commits August 29, 2024 14:27

Storybook fixes + upodate notice to use section

6aeceea

Update to new tokens package version

82a514f

melaniebmn requested changes Sep 4, 2024

View reviewed changes

daine reviewed Sep 4, 2024

View reviewed changes

PR feedback: Notice formatting + clean up

72f4217

ethanWallace requested a review from melaniebmn September 5, 2024 12:30

ethanWallace added 2 commits September 5, 2024 09:25

Remove unused line of CSS

6f30991

Add missing lang attribute in storybook

b456e62

daine requested changes Sep 5, 2024

View reviewed changes

PR feedback: fix typos

d6960b0

melaniebmn reviewed Sep 9, 2024

View reviewed changes

duboisp suggested changes Sep 16, 2024

View reviewed changes

ethanWallace added 3 commits September 26, 2024 10:47

First version of icon notice

5bb40b1

Update notice component to use icon instead of text label

21de54e

Merge main

39686eb

ethanWallace mentioned this pull request Oct 1, 2024

feat: Update notice tokens for new icon design cds-snc/gcds-tokens#330

Merged

Update spec unit tests

79d0574

ethanWallace requested review from melaniebmn and daine October 1, 2024 18:32

daine requested changes Oct 2, 2024

View reviewed changes

Update icon selection method

6823317

ethanWallace requested review from duboisp and daine October 2, 2024 12:29

ethanWallace mentioned this pull request Oct 2, 2024

feat: update notice tokens for new icon design cds-snc/gcds-tokens#332

Merged

Update tokens package

e93b58e

melaniebmn approved these changes Oct 3, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: New gcds-notice component #627

feat: New gcds-notice component #627

ethanWallace commented Aug 27, 2024 •

edited

Loading

melaniebmn left a comment

daine Sep 4, 2024

ethanWallace Sep 5, 2024 •

edited

Loading

daine Sep 5, 2024

daine Sep 5, 2024

daine Sep 5, 2024

daine Sep 5, 2024

melaniebmn left a comment

duboisp left a comment

duboisp Sep 16, 2024

duboisp Sep 16, 2024

ethanWallace Sep 17, 2024

daine left a comment

daine Oct 2, 2024

ethanWallace commented Oct 2, 2024

melaniebmn left a comment

	{/* Hidden colon to provide pause between caption and heading text for AT */}
	{/* Hidden colon to provide pause between caption and heading text for accessibility tree */}

	describe('gcds-alert', () => {
	describe('gcds-notice', () => {

feat: New gcds-notice component #627

Are you sure you want to change the base?

feat: New gcds-notice component #627

Conversation

ethanWallace commented Aug 27, 2024 • edited Loading

Summary | Résumé

melaniebmn left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ethanWallace Sep 5, 2024 • 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

melaniebmn left a comment

Choose a reason for hiding this comment

duboisp 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

daine left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ethanWallace commented Oct 2, 2024

melaniebmn left a comment

Choose a reason for hiding this comment

ethanWallace commented Aug 27, 2024 •

edited

Loading

ethanWallace Sep 5, 2024 •

edited

Loading