Create a Status Alert #42

fysheets · 2017-10-03T20:53:16Z

Create a new component for StatusAlert that is accessible and reusable.

coveralls · 2017-10-03T20:54:51Z

Coverage increased (+0.03%) to 98.537% when pulling efa09a564d4a4e6b058ccccf5bdc46be10d4f585 on fsheets/status-alert into 35d2bf8 on master.

coveralls · 2017-10-04T13:56:50Z

Coverage increased (+0.03%) to 98.537% when pulling 169b6f97869f2e7287d543c090e2609077cde8bc on fsheets/status-alert into 35d2bf8 on master.

coveralls · 2017-10-04T14:57:22Z

Coverage increased (+0.03%) to 98.537% when pulling 9e137c82aa5555e41d0fad4b1ecb18f9c8c0a9f5 on fsheets/status-alert into 35d2bf8 on master.

coveralls · 2017-10-04T16:02:31Z

Coverage increased (+0.03%) to 98.537% when pulling 5a400a98155f7243376a840238380216a25ce3cb on fsheets/status-alert into 35d2bf8 on master.

coveralls · 2017-10-04T18:54:23Z

Coverage increased (+0.7%) to 99.251% when pulling eda87287af292b23076e8b92ccfba714f1caa05f on fsheets/status-alert into 35d2bf8 on master.

coveralls · 2017-10-04T19:57:31Z

Coverage decreased (-6.6%) to 91.892% when pulling 059141eea65da7446ef59ab1558864382108041c on fsheets/status-alert into 35d2bf8 on master.

coveralls · 2017-10-04T19:59:18Z

Coverage decreased (-6.6%) to 91.892% when pulling 059141eea65da7446ef59ab1558864382108041c on fsheets/status-alert into 35d2bf8 on master.

coveralls · 2017-10-04T20:33:11Z

Coverage decreased (-7.2%) to 91.275% when pulling e39c0111f59eb89daefb2eb12335380071b30b41 on fsheets/status-alert into 35d2bf8 on master.

coveralls · 2017-10-04T21:51:17Z

Coverage decreased (-7.004%) to 91.503% when pulling 7533e1425789d8154f9ce80049fce995ba832b40 on fsheets/status-alert into 35d2bf8 on master.

coveralls · 2017-10-05T14:20:48Z

Coverage decreased (-6.5%) to 92.027% when pulling 72509e5a49163cb68baf55f260d330d9c1c9c857 on fsheets/status-alert into 35d2bf8 on master.

coveralls · 2017-10-05T15:30:46Z

Coverage decreased (-6.2%) to 92.308% when pulling 6a507569232dfcc317098d39fa29b08132fbf8f1 on fsheets/status-alert into 35d2bf8 on master.

coveralls · 2017-10-05T16:23:39Z

Coverage increased (+0.5%) to 99.038% when pulling 49426ed7f1212d1d8a22e48401bafc8edc17d1d8 on fsheets/status-alert into 35d2bf8 on master.

coveralls · 2017-10-05T16:35:05Z

Coverage decreased (-0.2%) to 99.038% when pulling 69ca19b308870eac1394dafeb2fd1ed437dfe000 on fsheets/status-alert into fd4b1bd on master.

coveralls · 2017-10-05T19:12:04Z

Coverage increased (+0.1%) to 99.353% when pulling f276c63b6e9559b4acf70644c39411c22dde536c on fsheets/status-alert into fd4b1bd on master.

arizzitano

This looks pretty good overall! Two main things: 1) I think onClose shouldn't be required if dismissible is false, 2) could you import and use the existing Paragon Button instead of the native <button />? The rest is just style suggestions.

arizzitano · 2017-10-06T15:08:43Z

src/StatusAlert/index.jsx

+  className: PropTypes.arrayOf(PropTypes.string),
+  dialog: PropTypes.oneOfType([PropTypes.string, PropTypes.element]).isRequired,
+  dismissible: PropTypes.bool,
+  onClose: PropTypes.func.isRequired,


So I'm team onClose required for the standard use case, but it should probably not be required if this is a dismissible alert. This would be a good place to use a custom prop validator -- maybe this package would be helpful? https://www.npmjs.com/package/react-proptype-conditional-require

That totally makes sense and that package looks interesting! I will read up and see if we can leverage it.

arizzitano · 2017-10-06T15:35:05Z

src/StatusAlert/index.jsx

+
+    if (dismissible) {
+      className.push(styles['alert-dismissible']);
+    }


Is it possible to inline this below within the classNames call (same way you did with the other conditional styles)? e.g. { [styles['alert-dismissible']]: dismissable }

Ah - the answer is most likely yes :) thanks!

arizzitano · 2017-10-06T16:36:20Z

src/StatusAlert/StatusAlert.stories.jsx

+            rel="noopener noreferrer"
+          >
+            Click me!
+          </a>


Really loving Paragon's quality cat content 😼

arizzitano · 2017-10-06T18:23:34Z

src/StatusAlert/index.jsx

+    let dismissButton = (<div />);
+    if (dismissible) {
+      dismissButton = (
+        <button


Can you use the Paragon Button component here instead of the native one?

Noting down our discussion -- the dismissible button formatting is generated from both the alert & close SASS files in order to render properly -- Will try to include the close partial into Button to see if it will continue to render correctly.

arizzitano · 2017-10-06T18:23:54Z

src/StatusAlert/index.jsx

+
+  renderDismissible() {
+    const { dismissible } = this.props;
+    let dismissButton = (<div />);


Why render this empty div if dismissible is false? Could we do something more along the lines of

return (dismissible) ? null : ( <Button // button props... /> );

I can definitely give this a go -- it didn't cross my mind that null was a valid thing that could be returned as part of a render.

@arizzitano I updated this to be null instead of the empty div. When I made my first attempt to turn it into a ternary operator it didn't render happily with the spanning over multiple lines. I will revisit this though when I test out the options to switch to the paragon Button from the above comment.

MichaelRoytman · 2017-10-06T19:44:41Z

src/StatusAlert/StatusAlert.stories.jsx

+      open
+    />
+  ))
+  .add('modal invoked via a button', () => (


I think you mean alert here.

Thank you Michael :)

MichaelRoytman · 2017-10-06T19:44:52Z

src/StatusAlert/StatusAlert.stories.jsx

+      dialog="Success! You triggered the alert!"
+    />
+  ))
+  .add('modal with a link', () => (


Alert here, too!

coveralls · 2017-10-06T21:01:14Z

Coverage increased (+0.1%) to 99.344% when pulling f820498b9cb7e191d09789a44d4b998ed31a29c1 on fsheets/status-alert into fd4b1bd on master.

coveralls · 2017-10-11T13:42:29Z

Coverage increased (+0.1%) to 99.344% when pulling bc15656e7ba29f96f7a8177bc6c5a34e5d9e99e4 on fsheets/status-alert into ece2748 on master.

coveralls · 2017-10-11T14:59:55Z

Coverage increased (+0.1%) to 99.344% when pulling 7a2235c1e531358aa056059808650962f197b91f on fsheets/status-alert into 4ebc1ab on master.

coveralls · 2017-10-11T19:18:18Z

Coverage increased (+0.1%) to 99.344% when pulling d07c6964123dc14c0e57978f285eb1a79960de2c on fsheets/status-alert into a4e492b on master.

coveralls · 2017-10-11T19:55:48Z

Coverage increased (+0.1%) to 99.34% when pulling a4650511bbebd252643611f4b9c94eec4bc5ceb9 on fsheets/status-alert into a4e492b on master.

coveralls · 2017-10-11T21:14:13Z

Coverage increased (+0.1%) to 99.34% when pulling 88edd5a9644f15633c28d439b34b26299202d936 on fsheets/status-alert into a4e492b on master.

fysheets · 2017-10-12T16:45:41Z

.storybook/__snapshots__/Storyshots.test.js.snap

+
+exports[`Storyshots StatusAlert alert invoked via a button 1`] = `
+<div>
+  <div


@arizzitano while playing with the screenreader yesterday, I think that I need to add an aria-hidden on the status alert that is tied with the "open" state field, but when I did my first attempt it didn't appear to work as I had expected. Advice on the logic that should go here?

Spoke with Mark -- he noted that we could leverage display: 'none' and display: 'block' for this instead of aria-hidden so I am looking into how to correctly integrate that logic into classNames

coveralls · 2017-10-12T19:19:29Z

Coverage increased (+0.1%) to 99.34% when pulling d828a7b084e0a2015ebb926ff0fa9ad6211b993b on fsheets/status-alert into a4e492b on master.

coveralls · 2017-10-12T20:37:54Z

Coverage increased (+0.1%) to 99.34% when pulling ff9a435e2c33a57150217be280c6d84f52412d5f on fsheets/status-alert into a4e492b on master.

coveralls · 2017-10-12T20:38:35Z

Coverage increased (+0.1%) to 99.34% when pulling ff9a435e2c33a57150217be280c6d84f52412d5f on fsheets/status-alert into a4e492b on master.

arizzitano

One request re: README but otherwise this looks great! 🚢

arizzitano · 2017-10-13T14:40:00Z

src/StatusAlert/README.md

+### `dismissible` (boolean; optional)
+`dismissible` specifies if the status alert will include the dismissible X button to close the alert. It defaults to true.
+
+### `onClose` (function; required)


Can you add a note about onClose not being required if dismissible is true?

For sure -- how does this look:

onClose (function; conditionally required)

onClose is a function that is called on close. It can be used to perform actions upon closing of the status alert, such as restoring focus to the previous logical focusable element. It is only required if dismissible is set to true and not required if the alert if not dismissible.

arizzitano · 2017-10-13T14:40:59Z

src/StatusAlert/index.jsx

+          [styles.show]: this.state.open,
+        })}
+        role="alert"
+        hidden={!this.state.open}


Great find re: hidden! 👏👏👏

coveralls · 2017-10-13T17:12:06Z

Coverage increased (+0.1%) to 99.34% when pulling ef19327c55078653fd423f72683462a9783caaa3 on fsheets/status-alert into a4e492b on master.

…sts. Bump to version 0.1.0.

coveralls · 2017-10-13T21:01:07Z

Coverage decreased (-7.004%) to 91.503% when pulling 7533e1425789d8154f9ce80049fce995ba832b40 on fsheets/status-alert into 35d2bf8 on master.

fysheets changed the title ~~WIP: Create a Status Alert~~ Create a Status Alert Oct 5, 2017

arizzitano suggested changes Oct 6, 2017

View reviewed changes

MichaelRoytman reviewed Oct 6, 2017

View reviewed changes

fysheets commented Oct 12, 2017

View reviewed changes

fysheets mentioned this pull request Oct 12, 2017

README Creations #46

Merged

arizzitano approved these changes Oct 13, 2017

View reviewed changes

Create new StatusAlert accessible component with focus options and te…

3e38121

…sts. Bump to version 0.1.0.

fysheets force-pushed the fsheets/status-alert branch from ef19327 to 3e38121 Compare October 13, 2017 20:47

Fix merge conflicts against master changes.

3049dae

fysheets merged commit c046e63 into master Oct 16, 2017

fysheets deleted the fsheets/status-alert branch October 16, 2017 13:27

PKulkoRaccoonGang pushed a commit to raccoongang/paragon that referenced this pull request Sep 20, 2022

fix: use pixel values for button border-radius attribute (openedx#42)

0658b72

Create a Status Alert #42

Create a Status Alert #42

Conversation

fysheets commented Oct 3, 2017 • edited Loading

coveralls commented Oct 3, 2017

coveralls commented Oct 4, 2017

coveralls commented Oct 4, 2017

coveralls commented Oct 4, 2017

coveralls commented Oct 4, 2017

coveralls commented Oct 4, 2017

coveralls commented Oct 4, 2017

coveralls commented Oct 4, 2017

coveralls commented Oct 4, 2017

coveralls commented Oct 5, 2017

coveralls commented Oct 5, 2017

coveralls commented Oct 5, 2017

coveralls commented Oct 5, 2017

coveralls commented Oct 5, 2017

arizzitano 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

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

coveralls commented Oct 6, 2017

coveralls commented Oct 11, 2017

coveralls commented Oct 11, 2017

coveralls commented Oct 11, 2017

coveralls commented Oct 11, 2017

coveralls commented Oct 11, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

coveralls commented Oct 12, 2017

coveralls commented Oct 12, 2017

coveralls commented Oct 12, 2017

arizzitano left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

onClose (function; conditionally required)

Choose a reason for hiding this comment

Choose a reason for hiding this comment

coveralls commented Oct 13, 2017

coveralls commented Oct 13, 2017 • edited Loading

fysheets commented Oct 3, 2017 •

edited

Loading

`onClose` (function; conditionally required)

coveralls commented Oct 13, 2017 •

edited

Loading