AR-572 Create Alert component #174

Jephuff · 2020-05-11T05:47:57Z

https://apollographql.atlassian.net/browse/AP-572

Create an Alert

jglovier

Looking good! Excited to see this coming along. ✨

Spotted just one small change...

src/Alert/Alert.tsx

jglovier · 2020-05-11T22:47:33Z

Also, I'm not sure how this should be implemented (whether via the SpaceKit component or on a per view basis), but the width of these cards should constrained to a max-width.

The Alert inline banners should flow with the content they are in, but toast cards will sit above the UI and slide in from off screen (sorry, that probably wasn't very clear from the design principles doc! 🙈) and generally should be constrained to a max-width in real use cases.

justinanastos

Hey @Jephuff, please hold on merging this until I review it this afternoon!

justinanastos

Hey @Jephuff, please hold on merging this until I review it this afternoon!

justinanastos

For all who are checking out this PR; I had a sync conversation about this with @Jephuff yesterday. My asks were to not extend from Card and to offer a color: PaletteColor prop that would be used to set all the necessary colors in favor of offering contexts for the colors that might not be consistent across all Apollo properties.

justinanastos · 2020-05-20T21:03:57Z

@AlexanderMann if you're cool working on this, definitely give it a shot. I'd start with a fresh PR and not extend from Card. Just style this on its own. We can decide soon if we want to abstract out the pattern where we have a rounded box with a shadow and background; that's not necessary here though, just copy-paste.

As for the colors, please accept a color: PaletteColor prop (color might be misleading, maybe accentColor?) and decide if you want that color to be for the text or the icon. Then use getOffsetInPalette to get a darker or lighter shade that you'd need for the icon/text.

I'd say give the component a title prop and then have the content rendered with children. I think that makes the props look like this:

interface AlertProps {
  /**
   * Color to apply to the (you choose, icon or text). We will programmatically
   * pick a darker/lighter shade for the other one (and that will presumably
   * change for a light and dark theme!) 
   */
  accentColor: PaletteColor;
  title: React.ReactNode;
}

I'm going to be off for the long weekend tomorrow but I'm happy to pop on and look at this if it's blocking.

cheapsteak · 2020-05-20T21:12:38Z

I'd start with a fresh PR and not extend from Card. Just style this on its own. We can decide soon if we want to abstract out the pattern where we have a rounded box with a shadow and background; that's not necessary here though, just copy-paste.

I have on occasion wished for a styled-component that looked like Card but didn't come with all the dressings that Card does, possibly a BaseCard or CardBase?

Jephuff · 2020-05-21T02:41:44Z

Updated the alert to not extend card, accept color as prop and accept icon as prop.
I also added a dark theme.

@cheapsteak I looked at making a common CardBase component, not a while lot is actually common between the 2

<section
    {...otherProps}
    css={{
      backgroundColor: colors.white,
      color: colors.black.base,
      boxShadow: `0 4px 8px 0 rgba(0, 0, 0, .04)`,
      borderStyle: "solid",
      borderWidth: 1,
      borderColor: colors.silver.dark,
    }}
  >{children}</section>

I'm going to copy-paste for now :D

cheapsteak · 2020-05-21T14:47:52Z

@cheapsteak I looked at making a common CardBase component, not a whole lot is actually common between the 2

<section
    {...otherProps}
    css={{
      backgroundColor: colors.white,
      color: colors.black.base,
      boxShadow: `0 4px 8px 0 rgba(0, 0, 0, .04)`,
      borderStyle: "solid",
      borderWidth: 1,
      borderColor: colors.silver.dark,
    }}
  >{children}</section>

I was hoping we'd be able to pull a border radius as well, but other than that that's pretty much what I was after

I wonder if you've caught this thread https://meteor.slack.com/archives/CJM405HJB/p1588779273039400 ?
Pulling in the Card component and overriding its overflow by & > div > div { ... } feels unpleasant when all he needed were borders and shadows

I would have just inlined the shadows myself, but my understanding is that we are heavily encouraging if not mandating that space kit be used whenever possible. That behooves us to try to provide a pleasant developer experience, and in my experience it's more pleasant to use something that gets me 80% of the way there even if I have to add the rest myself, than to have something that gets me 120% of the way there then I have to fight the framework to dispel the unwanted extras. Although, I guess this might be subjective?

Anyway, feel free to punt that from this PR as we can fairly say this is out of scope :)

Jephuff · 2020-05-21T15:06:15Z

Also, I'm not sure how this should be implemented (whether via the SpaceKit component or on a per view basis), but the width of these cards should constrained to a max-width.

The Alert inline banners should flow with the content they are in, but toast cards will sit above the UI and slide in from off screen (sorry, that probably wasn't very clear from the design principles doc! 🙈) and generally should be constrained to a max-width in real use cases.

My goal with this was to provide the UI to be used in different places like a Toast component (or modal, or inline). so I think max-width and if the card flows with the rest of the document would be determined by the usage 👍

jglovier

These are coming along nicely, but still noticing a few discrepancies between the implementation and the designs:

the border below the heading should only be used on cards with the expanded content version (also please note the content left margin differs slightly between these variants)
the heading bottom border on the dark mode cards should be grey-dark (differs from the light mode cards
the button the dark mode cards should use the dark theme buttons (I think they have a darker border color for dark contexts)

Let me know if you have any questions about the specifics of the designs. I'd be happy to go over them with you to clarify anything that may be confusing.

jglovier · 2020-05-21T17:21:05Z

My goal with this was to provide the UI to be used in different places like a Toast component (or modal, or inline). so I think max-width and if the card flows with the rest of the document would be determined by the usage

Just to clarify, this design is only intended for use as a toast message. By definition (Bootstrap example, Primer example, Android example) a toast is a temporary message that doesn't belong in the normal flow of UI and generally is triggered by an action the user has taken during their workflow, generally appearing from off-screen or floated into the view from an hidden state.

This component should never be used as a modal or inline alert.

Alert banners vs Alert toasts

The use case for inline alert banners differs from the toast message (which is why the design is styled explicitly different from toasts). Those should be displayed when something in the app, data, or user's account is in a state that requires an alert (vs being triggered by an immediate action).

For example:

deprecated fields will always show an inline warning alert next to them in the UI
when we are changing a feature and it may go away, we might show an inline alert in the UI until the feature is gone
if there is a problem with your account (like payment method missing) we might show an inline alert on the account page until it's resolved

Point being that inline alert banners will stay in the layout permanently until the issue is resolved, verses a toast message which appears momentarily until you dismiss it or reload the page.

Apologies if all of this wasn't very clear in the quip doc. 🙏

All that to say, keeping toast cards with a consistent width (and displaying them in the same place) will be an important part of their usage in the app. If the best thing to do is to implement that on a case by case basis (rather than in the component), that's cool. I just want to make sure the context for how these will be used is clear. 😄

cheapsteak · 2020-05-22T02:41:44Z

Glad we did a one-off for studio first so we can take the time to get this right 😄

justinanastos

@Jephuff this is great work. You've got test cases and explanations in the story and the API is solid. I asked for one API change, let me know how you feel about that.

Keep up the good work!

src/Alert/Alert.tsx

justinanastos · 2020-05-26T20:53:05Z

@Jephuff I also updated the title of this PR because that's what will show up in the change log 😃

justinanastos · 2020-05-26T20:54:29Z

@cheapsteak I agree with you on coming up with something to prevent us from having to re-invent the wheel for the rounded, bordered, shadowed edge of these components. I thought it was fine to keep outside the scope of this; you're right we'll totally need to do that at some point. I hadn't actually considered that it's something we'd want outside of Space Kit. If that's the case, I'd name it something like OutlinedBox or something.

…Unreachable

justinanastos

Thanks for making these changes. This is great. A few things:

I left a few comments labeled as minor. When I say minor (I really need to add either a contribution guide or a @justinanastos user manual) it means it's a change that I want you to make but I don't need to review it after so you can self-resolve the comment. If you disagree, which is totally fine, please continue the conversation!
Left a comment about the "inner" logic for our SVGs, I want that to merge here and I want to add something special to our svg → tsx conversion process so we don't need to use &.inner anywhere.

justinanastos · 2020-05-27T14:52:07Z

src/Alert/Alert.tsx

+  icon: PropTypes.element.isRequired,
+  headingAs: PropTypes.oneOfType([
+    PropTypes.element.isRequired,
+    PropTypes.string.isRequired as any, // Using PropTypes.string to match keyof JSX.IntrinsicElements


🤯 I didn't know we could bypass the TS check here with as any! Awesome.

justinanastos · 2020-05-28T17:24:20Z

src/Alert/Alert.story.mdx

@@ -42,7 +42,7 @@ Informational alerts present general info about something you are doing or worki
    <Alert
      onClose={() => undefined}
      color={colors.blue.base}
-      icon={<IconInfoSolid />}
+      icon={<IconInfoSolid css={{ "& .inner": { fill: colors.white } }} />}


This seems like a failing of our icon components that we should address soon.

src/Alert/Alert.tsx

justinanastos · 2020-05-28T17:32:09Z

src/shared/assertUnreachable.ts

+// @see https://www.typescriptlang.org/docs/handbook/advanced-types.html#exhaustiveness-checking
+/* istanbul ignore next */
+export function assertUnreachable(value: never): never {
+  throw new TypeError(`Unreachable value reached ${value}`);
+}


I'm glad you separated this out. I was going to ask you to add this to the excluded files in rollup so we don't export an assertUnreachable file, but our config already handles this 😃

space-kit/rollup.config.js

Lines 32 to 46 in b3fa01c

input: recursiveReaddirSync(path.resolve("src"))

// Don't build tests or storybook stories

.filter((filename) => !/\.(?:spec|story|stories)/.test(filename))

// Exclude specific directories

.filter(

(filename) =>

![

path.join("src", "icons", "scripts"),

path.join("src", "illustrations", "scripts"),

path.join("src", "shared"),

path.join("src", "AbstractTooltip"),

path.join("src", "MenuItemClickListener"),

path.join("src", "MenuConfig"),

].some((excludedPathname) => filename.includes(excludedPathname))

),

jglovier

@Jephuff these are coming along really nicely!! ✨👌✨

The components themselves all look as expected in Storybook, so I'm going to approve the PR to keep from blocking you any further. However, there are a few things I noticed that I'd like to point out and I'll leave it to you to decide whether they are worth changing in this PR or later.

So the first thing is that the Extended Alert variant looks just like the basic version because it doesn't use long text in it's example as is the intent for the card, and also it doesn't have any clarifying instructions below the heading as to it's purpose.

To avoid confusion around it's usage intentions, could we add a paragraph or two of text to the body of the example to display it's intended usage, and add a line below the heading to clarify? (Something like "The extended card variant is for rare use cases where a toast alert is the most appropriate format for explaining longer instructions."

The other thing I noticed is that in SpaceKit this component is listed as an Alert, but technically it's a specific type of alert (alert toast card) out of two currently designed for types (alert banner and alert toast cards). Is your intention to rename these to Alert Toast Cards or a similar designation after we add the alert banners?

Again, feel free to merge and keep rolling along and address these other two points later/elsewhere if you'd prefer. Just wanted to make sure these things are on your radar.

Jephuff · 2020-06-02T03:52:25Z

@jglovier Updated the text with some more explanation 👍
also that is a great point about the name, this will collide with alert banners. I've renamed it to AlertCard, @justinanastos does that sound good to you?

justinanastos

Sorry to go from a 👍 to asking for a change. The storybook story needs it's context of "success" and "info" removed. If we want to keep this documentation, it belongs in new components inside of Studio (formerly known as AGM/engine).

Also, a question on naming 😃

justinanastos · 2020-06-03T17:45:54Z

src/AlertCard/AlertCard.story.mdx

+      <br />
+      If a toast card’s content requires more than one paragraph, use the
+      expanded content toast variant.
+    </AlertCard>
+  </Story>
+</Preview>
+
+## Warning Alert
+
+Warning alerts present information about the action that is about to be taken that alerts the user to consequences of the action and asks for confirmation that the action still be taken in light of the consequences.
+
+<Preview>
+  <Story name="warning light">
+    <AlertCard
+      onClose={() => undefined}
+      color={colors.orange.base}
+      icon={<IconWarningSolid />}
+      heading="Warning alert"
+      actions={<Button size="small">Confirm</Button>}
+    >
+      exceededSubLimitAt is a deprecated field. You can use it but it may not
+      work as expected.
+    </AlertCard>
+  </Story>
+  <Story name="warning dark">
+    <AlertCard
+      onClose={() => undefined}
+      color={colors.orange.base}
+      icon={<IconWarningSolid />}
+      heading="Warning alert"
+      theme="dark"
+      actions={
+        <Button theme="dark" size="small">
+          Confirm
+        </Button>
+      }
+    >
+      exceededSubLimitAt is a deprecated field. You can use it but it may not
+      work as expected.
+    </AlertCard>
+  </Story>
+</Preview>
+
+## Error Alert
+
+Error alerts let the user know that something has gone wrong or is blocked in the given workflow they are trying to complete. When possible, error alerts should contain helpful information about what has happened to help unblock the user (such as steps to resolve or a link to a docs article).
+
+<Preview>
+  <Story name="error light">
+    <AlertCard
+      onClose={() => undefined}
+      color={colors.red.base}
+      icon={<IconErrorSolid />}
+      heading="Error alert"
+      style={{ marginBottom: 20 }}
+    >
+      Something is broken. You cannot perform this action at this time.
+    </AlertCard>
+  </Story>
+  <Story name="error dark">
+    <AlertCard
+      onClose={() => undefined}
+      color={colors.red.base}
+      icon={<IconErrorSolid css={{ "& .inner": { fill: colors.white } }} />}
+      heading="Error alert"
+      theme="dark"
+      style={{ marginBottom: 20 }}
+    >
+      Something is broken. You cannot perform this action at this time.
+    </AlertCard>
+  </Story>
+</Preview>
+
+## Success Alert
+
+Success alerts are confirmation that the action the user was trying to take has succeeded.
+
+<Preview>
+  <Story name="success light">
+    <AlertCard
+      onClose={() => undefined}
+      color={colors.green.base}
+      icon={<IconSuccessSolid />}
+      heading="Success alert"
+    >
+      A new schema registry has been created by timbotnik.
+    </AlertCard>
+  </Story>
+  <Story name="success dark">
+    <AlertCard
+      onClose={() => undefined}
+      color={colors.green.base}
+      icon={<IconSuccessSolid css={{ "& .inner": { fill: colors.white } }} />}
+      theme="dark"
+      heading="Success alert"
+    >
+      A new schema registry has been created by timbotnik.
+    </AlertCard>
+  </Story>
+</Preview>


I missed this in my review, so I'm sorry. We're not going to share contextual components as Space Kit ideas, just components that can be configured. Instead of demonstrating like this, demonstrate how the component can be configured.

I can definitely remove the concepts of error/warning/success/error.
before I do that, @jglovier I think you might have had thoughts on this?

justinanastos · 2020-06-03T17:46:45Z

src/AlertCard/AlertCard.tsx

@@ -0,0 +1,234 @@
+/** @jsx jsx */


I'm commenting here just so the conversation can be threaded. I'm confused as to why this is named AlertCard. Which item in the design principals docs is this implementing and what naming conflict was there?

https://apollographql.quip.com/AgFcAwybcEWN/Alerts-design-principles

These are to be used in the Alert toast cards.
Joel pointed out the conflict

The other thing I noticed is that in SpaceKit this component is listed as an Alert, but technically it's a specific type of alert (alert toast card) out of two currently designed for types (alert banner and alert toast cards). Is your intention to rename these to Alert Toast Cards or a similar designation after we add the alert banners?

We will need an AlertBanner soon so AlertCard and AlertBanner seemed preferable to Alert and AlertBanner

…nent

justinanastos

LGTM! Great work @Jephuff !

apollo-bot2 · 2020-06-12T18:23:02Z

🚀 PR was released in v5.6.0 🚀

jglovier · 2020-06-12T19:39:22Z

extend Card to make an Alert component

61732c4

Jephuff added the minor Increment the minor version when merged label May 11, 2020

Jephuff requested review from justinanastos, jglovier, cheapsteak, daniman and jgzuke May 11, 2020 14:03

jglovier suggested changes May 11, 2020

View reviewed changes

src/Alert/Alert.tsx Outdated Show resolved Hide resolved

src/Alert/Alert.tsx Outdated Show resolved Hide resolved

justinanastos reviewed May 12, 2020

View reviewed changes

justinanastos self-requested a review May 12, 2020 17:28

justinanastos suggested changes May 13, 2020

View reviewed changes

Jephuff added 6 commits May 20, 2020 21:11

use orange instead of yellow

b9b82c6

copy card code

049ebc2

accept color and icon instead of level

195bb0e

add dark theme

53cfae6

update comments

3f33d1e

Merge branch 'master' into jeffrey/alert-component

06079ac

Jephuff added 2 commits May 20, 2020 22:43

revert changes to Card

bc47dd9

remove any type

c1bd2c0

Jephuff requested review from jglovier and justinanastos May 21, 2020 15:28

jglovier suggested changes May 21, 2020

View reviewed changes

add extended variant

8012ecd

justinanastos suggested changes May 26, 2020

View reviewed changes

src/Alert/Alert.tsx Outdated Show resolved Hide resolved

src/Alert/Alert.tsx Outdated Show resolved Hide resolved

src/Alert/Alert.tsx Outdated Show resolved Hide resolved

src/Alert/Alert.tsx Outdated Show resolved Hide resolved

src/Alert/Alert.tsx Outdated Show resolved Hide resolved

justinanastos changed the title ~~Extend Card to make an Alert component~~ AR-572 Extend Card to make an Alert component May 26, 2020

justinanastos changed the title ~~AR-572 Extend Card to make an Alert component~~ AR-572 Create Alert component May 26, 2020

Jephuff added 3 commits May 26, 2020 22:19

switch Icon to element instead of component

1dff30a

assert unexpected theme values are unreachable and make common assert…

d3a7d75

…Unreachable

add proptype for headingAs

c47b8db

Jephuff requested review from justinanastos and jglovier May 27, 2020 03:00

style inner svg shape

d84a833

justinanastos approved these changes May 28, 2020

View reviewed changes

Jephuff added 2 commits May 28, 2020 14:22

guard all theme usages

11716d3

use font helper

34622f4

jglovier approved these changes Jun 1, 2020

View reviewed changes

Jephuff added 2 commits June 1, 2020 23:31

update copy in alert story

0ede668

rename Alert to AlertCard

8c31c3c

justinanastos suggested changes Jun 3, 2020

View reviewed changes

Jephuff added 2 commits June 11, 2020 16:59

switch back to type of alert prop

2b26b86

Merge remote-tracking branch 'origin/master' into jeffrey/alert-compo…

8b959a4

…nent

Jephuff requested a review from justinanastos June 11, 2020 21:07

justinanastos approved these changes Jun 12, 2020

View reviewed changes

Jephuff merged commit da97a2f into master Jun 12, 2020

Jephuff deleted the jeffrey/alert-component branch June 12, 2020 18:19

apollo-bot2 added the released This issue/pull request has been released. label Jun 12, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

AR-572 Create Alert component #174

AR-572 Create Alert component #174

Jephuff commented May 11, 2020 •

edited by justinanastos

Loading

jglovier left a comment

jglovier commented May 11, 2020

justinanastos left a comment

justinanastos left a comment

justinanastos left a comment

justinanastos commented May 20, 2020

cheapsteak commented May 20, 2020 •

edited

Loading

Jephuff commented May 21, 2020

cheapsteak commented May 21, 2020 •

edited

Loading

Jephuff commented May 21, 2020

jglovier left a comment

jglovier commented May 21, 2020 •

edited

Loading

cheapsteak commented May 22, 2020

justinanastos left a comment

justinanastos commented May 26, 2020

justinanastos commented May 26, 2020

justinanastos left a comment

justinanastos May 27, 2020

justinanastos May 28, 2020

justinanastos May 28, 2020

jglovier left a comment •

edited

Loading

Jephuff commented Jun 2, 2020

justinanastos left a comment

justinanastos Jun 3, 2020

Jephuff Jun 3, 2020

justinanastos Jun 3, 2020

Jephuff Jun 3, 2020

justinanastos left a comment

apollo-bot2 commented Jun 12, 2020

jglovier commented Jun 12, 2020

	input: recursiveReaddirSync(path.resolve("src"))
	// Don't build tests or storybook stories
	.filter((filename) => !/\.(?:spec\|story\|stories)/.test(filename))
	// Exclude specific directories
	.filter(
	(filename) =>
	![
	path.join("src", "icons", "scripts"),
	path.join("src", "illustrations", "scripts"),
	path.join("src", "shared"),
	path.join("src", "AbstractTooltip"),
	path.join("src", "MenuItemClickListener"),
	path.join("src", "MenuConfig"),
	].some((excludedPathname) => filename.includes(excludedPathname))
	),

AR-572 Create Alert component #174

AR-572 Create Alert component #174

Conversation

Jephuff commented May 11, 2020 • edited by justinanastos Loading

jglovier left a comment

Choose a reason for hiding this comment

jglovier commented May 11, 2020

justinanastos left a comment

Choose a reason for hiding this comment

justinanastos left a comment

Choose a reason for hiding this comment

justinanastos left a comment

Choose a reason for hiding this comment

justinanastos commented May 20, 2020

cheapsteak commented May 20, 2020 • edited Loading

Jephuff commented May 21, 2020

cheapsteak commented May 21, 2020 • edited Loading

Jephuff commented May 21, 2020

jglovier left a comment

Choose a reason for hiding this comment

jglovier commented May 21, 2020 • edited Loading

cheapsteak commented May 22, 2020

justinanastos left a comment

Choose a reason for hiding this comment

justinanastos commented May 26, 2020

justinanastos commented May 26, 2020

justinanastos left a comment

Choose a reason for hiding this comment

justinanastos May 27, 2020

Choose a reason for hiding this comment

justinanastos May 28, 2020

Choose a reason for hiding this comment

justinanastos May 28, 2020

Choose a reason for hiding this comment

jglovier left a comment • edited Loading

Choose a reason for hiding this comment

Jephuff commented Jun 2, 2020

justinanastos left a comment

Choose a reason for hiding this comment

justinanastos Jun 3, 2020

Choose a reason for hiding this comment

Jephuff Jun 3, 2020

Choose a reason for hiding this comment

justinanastos Jun 3, 2020

Choose a reason for hiding this comment

Jephuff Jun 3, 2020

Choose a reason for hiding this comment

justinanastos left a comment

Choose a reason for hiding this comment

apollo-bot2 commented Jun 12, 2020

jglovier commented Jun 12, 2020

Jephuff commented May 11, 2020 •

edited by justinanastos

Loading

cheapsteak commented May 20, 2020 •

edited

Loading

cheapsteak commented May 21, 2020 •

edited

Loading

jglovier commented May 21, 2020 •

edited

Loading

jglovier left a comment •

edited

Loading