Improved dialog and docs + button loading #134

jeroenransijn · 2018-02-15T00:51:04Z

This PR is based on top of #127, #128. The other PR was still based on the old mono-repo, so I redid most what was in that PR and made some additional changes and tweaks.

This is a breaking change

This PR contains

Dialog API improvements
More Dialog stories
Dialog documentation
Button now has a isLoading prop
Minor changes in content for Table and Buttons docs.

Props and default props

static propTypes = {
  /**
   * Children can be a node or a function accepting `({ close })`.
   * See an example to understand how this works.
   */
  children: PropTypes.oneOfType([PropTypes.node, PropTypes.func]).isRequired,

  /**
   * When true, the dialog is shown.
   */
  isShown: PropTypes.bool,

  /**
   * Title of the Dialog. Titles should use Title Case.
   */
  title: PropTypes.node,

  /**
   * When true, the header with the title and close icon button is shown.
   */
  hasHeader: PropTypes.bool,

  /**
   * When true, the footer with the cancel and confirm button is shown.
   */
  hasFooter: PropTypes.bool,

  /**
   * When true, the cancel button is shown.
   */
  hasCancel: PropTypes.bool,

  /**
   * Function that will be called when the exit transition is complete.
   */
  onCloseComplete: PropTypes.func,

  /**
   * Function that will be called when the enter transition is complete.
   */
  onOpenComplete: PropTypes.func,

  /**
   * Function that will be called when the confirm button is clicked.
   * This does not close the Dialog. A close function will be passed
   * as a paramater you can use to close the dialog.
   *
   * `onConfirm={(close) => close()}`
   */
  onConfirm: PropTypes.func,

  /**
   * Label of the confirm button.
   */
  confirmLabel: PropTypes.string,

  /**
   * The type of the message.
   */
  type: PropTypes.oneOf(['default', 'danger']),

  /**
   * When true, the confirm button is set to loading.
   */
  isConfirmLoading: PropTypes.bool,

  /**
   * When true, the confirm button is set to disabled.
   */
  isConfirmDisabled: PropTypes.bool,

  /**
   * Function that will be called when the cancel button is clicked.
   * This closes the Dialog by default.
   *
   * `onCancel={(close) => close()}`
   */
  onCancel: PropTypes.func,

  /**
   * Label of the cancel button.
   */
  cancelLabel: PropTypes.string,

  /**
   * Width of the Dialog.
   */
  width: PropTypes.oneOfType([PropTypes.string, PropTypes.number]),

  /**
   * The space above the dialog.
   * This offset is also used at the bottom when there is not enough space
   * available on screen — and the dialog scrolls internally.
   */
  topOffset: PropTypes.oneOfType([PropTypes.string, PropTypes.number]),

  /**
   * The min height of the body content.
   * Makes it less weird when only showing little content.
   */
  minHeightContent: PropTypes.oneOfType([PropTypes.string, PropTypes.number]),

  /**
   * Props that are passed to the dialog container.
   */
  containerProps: PropTypes.object
}

static defaultProps = {
  isShown: false,
  hasHeader: true,
  hasFooter: true,
  hasCancel: true,
  type: 'default',
  width: 560,
  topOffset: '12vh',
  minHeightContent: 80,
  confirmLabel: 'Confirm',
  isConfirmLoading: false,
  isConfirmDisabled: false,
  cancelLabel: 'Cancel',
  onCancel: close => close(),
  onConfirm: close => close()
}

Design Examples

These examples don‘t reflect the latest state of the component.

Rowno · 2018-02-15T01:31:47Z

docs/src/components/ComponentReadme.js

@@ -1,3 +1,4 @@
+/* eslint-disable react/no-danger */


Why add this? I don't see dangerouslySetInnerHTML anywhere?
https://github.com/yannickcr/eslint-plugin-react/blob/master/docs/rules/no-danger.md

Oh yeah removed that later.

Rowno · 2018-02-15T01:33:35Z

docs/src/css/component-readme.css

-      }
-    }
+  &:hover {
+    text-decoration: underline;


Might want a focus style too? 😉

I was too lazy.

Rowno · 2018-02-15T01:34:02Z

docs/src/css/main-layout.css

  flex: 1;
  display: flex;
-  background-color: var(--neutral-3);
+  /* background-color: var(--neutral-3); */


Probably should just delete it.

Rowno · 2018-02-15T01:37:30Z

src/buttons/docs/LoadingManager.js

+  render() {
+    return this.props.children({
+      isLoading: this.state.isLoading,
+      setLoading: () => {


You should really make this a method on the class and keep the JSX clean.

Rowno · 2018-02-15T01:38:22Z

src/buttons/stories/index.stories.js

+  render() {
+    return this.props.children({
+      isLoading: this.state.isLoading,
+      setLoading: () => {


Rowno · 2018-02-15T01:44:22Z

src/dialog/docs/index.js

+        BlueprintJS
+      </a>{' '}
+      pointed out in their documentation that &ldquo;modal&rdquo; is a misnomer
+      for &ldquo;dialog&rdquo;.


Use “ and ” instead. Unicode is your friend. 😉

Rowno · 2018-02-15T01:45:53Z

src/dialog/src/Dialog.js

+     * Children can be a node or a function accepting `({ close })`.
+     * See an example to understand how this works.
+     */
+    children: PropTypes.oneOfType([PropTypes.node, PropTypes.func]),


Might be worth making children required because a dialog without children is pretty useless. 😛

Rowno · 2018-02-15T01:46:31Z

src/dialog/src/Dialog.js

+     * Passing this object will show the button and cancel button.
+     * You should pass `children` and `onClick`.
+     */
+    primaryButton: PropTypes.object,


So it can't be boolean?

Most of the time you wouldn't really want to pass props to the primary button though?

Then you'll end up with lots of code like primaryButton={{}}.

You have to pass the onClick handler

And you need to pass in children to give it a label. You always have to define something right?

Rowno · 2018-02-15T01:49:06Z

src/dialog/src/Dialog.js

+     * This offset is also used at the bottom when there is not enough space
+     * available on screen — and the Dialog scrolls internally.
+     */
+    topOffset: PropTypes.oneOfType([PropTypes.string, PropTypes.number]),


This can't be a number because I don't think unitless values will work in the CSS calc below.

Ah, we need to do add px then.

Rowno · 2018-02-15T01:51:00Z

src/dialog/src/Dialog.js

+                          : close()
+                      }
+                    />
+                    {hideCancelButton ? null : (


Just do {!hideCancelButton && (.

* improved dialog and docs + button loading * fixes * fix build * more explicit API * intent => type keep consistent with Alert * update story and docs * removed old showCancel * docs fixes

improved dialog and docs + button loading

04b2ecf

jeroenransijn added documentation improvement labels Feb 15, 2018

jeroenransijn requested a review from Rowno February 15, 2018 00:51

Rowno suggested changes Feb 15, 2018

View reviewed changes

jeroenransijn added 7 commits February 14, 2018 18:20

fixes

994caec

fix build

b7409dd

more explicit API

bd184c3

intent => type keep consistent with Alert

849b595

update story and docs

5f87fdd

removed old showCancel

f909803

docs fixes

e3b65c6

jeroenransijn merged commit 2919112 into master Feb 15, 2018

jeroenransijn deleted the improve-dialog-2 branch February 15, 2018 21:39

Improved dialog and docs + button loading #134

Improved dialog and docs + button loading #134

Uh oh!

Conversation

jeroenransijn commented Feb 15, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

This PR contains

Props and default props

Design Examples

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

jeroenransijn commented Feb 15, 2018 •

edited

Loading