Feature: #41 Alert. #60

damontgomery · 2021-07-16T21:08:21Z

Description

Adds an alert. See #41.

Testing notes

Visit http://localhost:6006/?path=/docs/molecules-alert--information and similar examples.

Example

Notes

Took styling and types mostly from #41.

This doesn't render an icon yet since there are still some open questions around that.

src/components/base/outline-alert/outline-alert.ts

madhaze · 2021-07-19T23:19:51Z

src/components/base/outline-alert/outline-alert.ts

+          : html``}
+        ${this.size === 'large'
+          ? html`
+              <div id="header">


Should this use the <outline-header>

Thanks for the question. I'm not sure.

In the storybook examples I left it as a slot, so we could pass in an <outline-header> if desired.

The trade-off there seems to be a little more work on the consumer of the component for the flexibility to define the header level / visual level.

I suppose we could always look to see if the slotted component was an <outline-header>, text, or something else and then only wrap text in the <outline-header>. What levels would we use then as defaults?

Anyone have ideas?

Good point. I saw the this.statusType and was thinking that was the text that was passed, but realize that is just a placeholder and we should be using content in the slot so that might make more sense. One reason I guess would be to keep all headings the same style.

I agree with Chris on this one, I think this should be an outline-heading with the statusType rendered inside of it. I don't think there is much benefit to allowing a consumer to place their own element here. If someone wanted to do that they could extend this element and overwrite the largeTemplate() method I'd mentioned above.

After a while on a project with the enhanced editor I think the slot is a better idea.

madhaze · 2021-07-20T06:30:16Z

We should also look at adding the ability to close alerts. If these might be used to display an alert dynamically after page load we should look into supporting aria-live polite

damontgomery · 2021-07-20T14:47:36Z

We should also look at adding the ability to close alerts. If these might be used to display an alert dynamically after page load we should look into supporting aria-live polite

Thanks, Chris!

I added this idea and the icon TODO to this story (#67) in case we want to implement the basic version first and then add enhancements.

This seems more explicit.

damontgomery · 2021-07-28T14:23:43Z

Added an automated test. This should be ready for review. :)

…erties.

mabry1985

The component overall looks good, I requested a few changes here that are mainly refactors and clean up.

src/components/base/outline-alert/outline-alert.ts

mabry1985 · 2021-07-30T00:51:11Z

src/components/base/outline-alert/outline-alert.ts

+          : html``}
+        ${this.size === 'large'
+          ? html`
+              <div id="header">


I agree with Chris on this one, I think this should be an outline-heading with the statusType rendered inside of it. I don't think there is much benefit to allowing a consumer to place their own element here. If someone wanted to do that they could extend this element and overwrite the largeTemplate() method I'd mentioned above.

src/components/base/outline-alert/outline-alert.stories.ts

damontgomery · 2021-08-10T20:51:33Z

src/components/base/outline-alert/outline-alert.ts

+  isInteractive = false;
+
+  @property({ type: Boolean })
+  shouldShowIcon = true;


This probably needs to be shouldHideIcon because of the way attributes work in the DOM. The presence of an attribute means true, so there is no way to set a false value if the default is true. :(

So, all boolean attributes probably need to default to false.

Interesting 🤯

LeeMellon · 2021-08-12T17:50:59Z

src/components/base/outline-alert/outline-alert.ts

+  render(): TemplateResult {
+    // The `body` wrapper is used to avoid styles (like border) that are preventing us from styling `:host`.
+    return html`
+      <div id="body" role="${this.isInteractive ? 'alertdialog' : 'alert'}">


I think this is going to require some more 'plumbing'.
Looks like you're going to need for 'alertdialog':

aria-label/ledby

focusable control/focused close button.

trapped/wrapping tab order.
https://developer.mozilla.org/enUS/docs/Web/Accessibility/ARIA/ARIA_Techniques/Using_the_alertdialog_role

I see the SB example, but we can't always be sure that the consumer will pass the requirements.

Thanks for the links! Maybe we can remove this feature for now and add a todo.

I think you could change isInteractive to something like alertDialogue: string and if it's there pass that to an aria-label attrib, and then set the role to "alertdialog". Just a thought to keep the functionality. Also we may just need to have a focusable close button here either way.

LeeMellon · 2021-08-12T18:04:55Z

src/components/base/outline-alert/outline-alert.ts

+export type AlertStatusType = typeof alertStatusTypes[number];
+
+// This can be useful for testing.
+export interface OutlineAlertInterface extends HTMLElement {


I like this a lot and think it should become a standard.

LeeMellon · 2021-08-12T18:12:52Z

src/components/base/outline-alert/outline-alert.ts

+  @property({ type: Boolean })
+  isInteractive = false;
+
+  @property({ type: Boolean })


Even if they're simple/obvious let's keep the standard of docs for every/state/method.

Yeah I agree, I've been doing this and reflecting that comment in the story controls. I'd like to look into creating a method for using those JSDocs to populate the storybook control info for us. Stencil does something similar.

I think for Storybook, especially, it's important to be explicit with the comments because there are QA, design, clients, etc. who are going to be seeing these pages. Its more user friendly and accessible is my argument.

A Storybook style guide is something I'd love to get nailed down soon, I'm just trying to figure out the best way to write them still too. Would be a good discussion for guild.

LeeMellon · 2021-08-12T18:39:54Z

src/components/base/outline-alert/outline-alert.stories.ts

+  `;
+};
+
+export const Information = Template.bind({});


I don't think we need a SB example for every possible iteration/variant.

I think it's beneficial to write stories for every visual representation of a component like we do with the button component, especially as we implement VRT. It would be good to detect if something you did broke a layout of a component in a specific state.

I'm down to have this discussion in a larger setting, but for me SB serves t purposes. 1: To show what the component can do. 2: To show how to use the component.
I think 1 is taken care of by the controls we take the time to set up. So the number of variants in a story should be dictated by the different ways the component can be used. EX: the alertdialog option here.

@LeeMellon , I think that's an interesting point.

This is the first time I've used storybook, so the patterns are new to me. I can see the point of view that a smaller number of stories may be easier for an implementer since it focuses their attention.

If it's helpful, this is what I've used Storybook for so far.

I write the storybook page first with examples of the variations that I understand need to be represented. In the alert, this means one of each status type. There is some level of feeling things out here. On a current project, we have an icon that can show 1800 icons and I don't have 1800 stories. So, I think 4 statuses makes sense to me as stories, but 1800 doesn't. I made another component to show all the colors / icons and those have their own component pages. In development, I was settings CSS classes per status type, but there were no CSS changes for those icons. Meaning, it was possible for me to break a single status type, but not a single icon.

If there is JS interaction, I may write the Jest tests at this point too.

Then, I start implementing the component itself. At this time, I'm watching those stories to see the components change and gradually get the features that I expect. Most of the time I'm looking at the docs pane, or for interaction / screen reader the iframe example.

If I forgot an example, I'll add more stories to capture those use cases.

... And I don't really use the controls much.

I agree with @mabry1985, that for visual regression testing, some of the additional variations may be helpful since the tools will default to one screenshot of each.

Might be a good team discussion or one for Github discussions.

LeeMellon · 2021-08-12T18:40:03Z

src/components/base/outline-alert/outline-alert.ts

+import componentStyles from './outline-alert.css.lit';
+import { OutlineElement } from '../outline-element/outline-element';
+
+export const alertSizes = ['small', 'large'] as const;


I think this is over complicating things. I think you want to replace these two lines with export type AlertSize = 'small' | 'large'

Thanks for the review @LeeMellon. Going to look into all this in more detail when I get a chance.

The attempt here was to provide an array of values used by Storybook to populate the select box and provide the type in this file for TS checking.

Is there a way to do both of those in one place? This pattern I found somewhere, but was new to me. :) The best I could find to avoid copy & paste issues.

No this is good, he's using const assertion here. I just figured out how to do this myself this week, it allows us to create a type and capture it in a variable in one go.

Thanks for the explanation, @mabry1985! And the link to where it's used.

Thanks both of you. Using this regularly now.

src/components/base/outline-alert/outline-alert.stories.ts

himerus · 2021-10-07T14:35:23Z

I know this one has been sitting a while, but I'm about ready to start the "mass merge". However, I want to hold on this one and update the outline-icon in #118. I figure there will need to be a minor adjustment to this MR then.

damontgomery added 7 commits July 16, 2021 14:10

41: Basic unstyled alert.

cae4905

41: Default header based on type.

b65fad5

41: Alert basic styling.

24dab7b

41: Add placeholder for icon.

b769038

41: Clean up stories.

a238e1d

41: Remove unused decorator.

e9ae7f7

41: Add story with link.

99be962

damontgomery added the Work In Progress label Jul 16, 2021

41: Add accessibility roles based on interactivity.

693c6ca

damontgomery removed the Work In Progress label Jul 16, 2021

41: Add information about the header slot.

646a156

damontgomery mentioned this pull request Jul 16, 2021

Alert Component #41

Closed

41: Refactor to move properties near where they are relevant.

2885dd9

damontgomery force-pushed the feature/41-alert branch from 22718b1 to 2885dd9 Compare July 16, 2021 21:26

damontgomery added 5 commits July 16, 2021 16:28

41: Simplify the header capitalization by using CSS.

57ce07a

41: Fix storybook issue with booleans.

5de4065

41: Fix booleans in storybook using simple Lit boolean syntax.

95a531f

41: Move simple functions into template.

f756efa

Merge branch 'next' into feature/41-alert

78c7cb2

madhaze reviewed Jul 19, 2021

View reviewed changes

src/components/base/outline-alert/outline-alert.ts Outdated Show resolved Hide resolved

madhaze reviewed Jul 19, 2021

View reviewed changes

damontgomery mentioned this pull request Jul 20, 2021

Enhancement: Alert #67

Closed

damontgomery added 2 commits July 20, 2021 10:00

41: Move properties to the top to match style expectations.

0400552

41: Use null when returning empty templates.

c6b73de

This seems more explicit.

damontgomery added the Needs Work label Jul 28, 2021

damontgomery added 3 commits July 28, 2021 09:05

Merge branch 'next' into feature/41-alert

30cf5fa

docs: 41: Update how we show the booleans.

9b17e1f

test: 41: Simple slotting test for outline alert.

704e602

chore: 41: Add interface for alert element.

0e32697

damontgomery added Ready for Review and removed Needs Work labels Jul 28, 2021

chore: 41: Reduce copy & paste in type definition for enumerable prop…

532d22c

…erties.

damontgomery force-pushed the feature/41-alert branch from 3682ca8 to 532d22c Compare July 28, 2021 18:40

mabry1985 suggested changes Jul 30, 2021

View reviewed changes

mabry1985 mentioned this pull request Jul 30, 2021

Feature: #45 Modal. #49

Merged

damontgomery commented Aug 10, 2021

View reviewed changes

LeeMellon reviewed Aug 12, 2021

View reviewed changes

mabry1985 reviewed Aug 13, 2021

View reviewed changes

src/components/base/outline-alert/outline-alert.stories.ts Show resolved Hide resolved

LeeMellon approved these changes Sep 16, 2021

View reviewed changes

himerus added the hacktoberfest-accepted label Oct 7, 2021

himerus added this to In Progress in Component Creation Initiative Oct 7, 2021

himerus merged commit fe48587 into next Oct 20, 2021

himerus deleted the feature/41-alert branch October 20, 2021 13:15

himerus removed the Ready for Review label Jan 6, 2022

himerus moved this from In Progress to Done in Component Creation Initiative Mar 31, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Feature: #41 Alert. #60

Feature: #41 Alert. #60

damontgomery commented Jul 16, 2021 •

edited by gitpod-io bot

madhaze Jul 19, 2021

damontgomery Jul 20, 2021

madhaze Jul 20, 2021

mabry1985 Jul 30, 2021 •

edited

LeeMellon Sep 16, 2021

madhaze commented Jul 20, 2021

damontgomery commented Jul 20, 2021

damontgomery commented Jul 28, 2021

mabry1985 left a comment

mabry1985 Jul 30, 2021 •

edited

damontgomery Aug 10, 2021

mabry1985 Aug 13, 2021

LeeMellon Aug 12, 2021

damontgomery Aug 12, 2021

LeeMellon Aug 13, 2021

LeeMellon Aug 12, 2021

LeeMellon Aug 12, 2021

mabry1985 Aug 13, 2021

LeeMellon Aug 12, 2021

mabry1985 Aug 13, 2021

LeeMellon Aug 13, 2021

damontgomery Aug 16, 2021

LeeMellon Aug 12, 2021

damontgomery Aug 12, 2021

mabry1985 Aug 13, 2021

damontgomery Aug 16, 2021

LeeMellon Sep 16, 2021

himerus commented Oct 7, 2021

Feature: #41 Alert. #60

Feature: #41 Alert. #60

Conversation

damontgomery commented Jul 16, 2021 • edited by gitpod-io bot

Description

Testing notes

Example

Notes

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mabry1985 Jul 30, 2021 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

madhaze commented Jul 20, 2021

damontgomery commented Jul 20, 2021

damontgomery commented Jul 28, 2021

mabry1985 left a comment

Choose a reason for hiding this comment

mabry1985 Jul 30, 2021 • 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

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

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

himerus commented Oct 7, 2021

damontgomery commented Jul 16, 2021 •

edited by gitpod-io bot

mabry1985 Jul 30, 2021 •

edited

mabry1985 Jul 30, 2021 •

edited