Dialog - Add Theme and ActionHeader #1209

saedar · 2018-02-05T20:01:40Z

Resolves: #1157

Summary

Dialog has been updated to consume ActionHeader. I have also added theme variables to Dialog, Modal, and ModalOverlay.

Deployment URL

https://terra-core-deployed-pr-1209.herokuapp.com/static/#/site/dialog

I also updated Dialog to use subcomponents for body and footer. Since I am having to pass the header/footer/body content to ContentContainer via props, I was unsure if how I have it currently working is the best approach. If not, I welcome tips on a better path.

EDIT: Subcomponent implementation has been removed for simplicity.

@cerner/terra

… Added new example. Added action header.

saedar · 2018-02-05T20:04:27Z

packages/terra-dialog/docs/README.md

 ```

+Note: This component has an i18n dependency that requires setup per [these steps](https://github.com/cerner/terra-core/#internationalization-i18n).


When trying to get ActionHeader working, I ran into issues because I didn't realize the need for Base as an ancestor. I wanted to include a link and brief description here to help future consumers avoid this confusion, too. Any tips on better wording appreciated.

saedar · 2018-02-05T20:05:37Z

packages/terra-dialog/tests/jest/Dialog.test.jsx

-    expect(wrapper).toMatchSnapshot();
-  });
-
-  it('should render header & footer', () => {


Talked with Neal and Brett last week. Footer is required, so don't need the special test cases with and without.

JakeLaCombe · 2018-02-05T21:15:12Z

packages/terra-dialog/docs/README.md

@@ -13,9 +13,22 @@ Dialogs are temporary views that can be used in a myriad of ways. Dialogs have t
 import React from 'react';
 import Dialog from 'terra-dialog';

-<Dialog header="Header Content" footer="Footer Content">some body content</Dialog>
+const header = 'Header Content';
+


It'd be great if we can get this API consistent. Ex: Since header doesn't require a wrapper, do we need them for body and footer?

I suppose it depends on what you mean by "need." I didn't create one for header because ActionHeader is already the wrapper.

The wrappers were suggested by Brett, so I will discuss this with him when he gets in.

I'm thinking put the wrappers inside the Dialog, and the content will just populate the wrappers in the dialog.

Yeah, thinking on this some more that makes sense. For the dialog subcomponets, like<Dialog.Body>, these will switch to an internal implementation. Likely <div className={dialogBodyClasses}>{dialogBody}</div>

JakeLaCombe · 2018-02-05T21:17:05Z

packages/terra-dialog/src/Dialog.jsx

+   * The footer element to be placed within the footer area of the dialog.
+   * The footer content area will not display if this prop is not provided. UNDER EVALUATION
+   */
+  footer: PropTypes.node.isRequired,


Remove the isRequired part, since it doesn't sound like it's required.

Actually, I forgot to update the doc. The footer is required now.

JakeLaCombe · 2018-02-05T21:17:21Z

packages/terra-dialog/src/Dialog.jsx

+  footer: PropTypes.node.isRequired,
+  /**
+   * The children to be placed within the main content area of the dialog.
+   */
  children: PropTypes.node.isRequired,


I'd make this optional as well to be honest.

What is a dialog with no content? Seems...wrong.

It does and I'm sure it's an unlikely case, but it's still an edge case that I'd feel safer having than not. I know for table cells that api is a little confusing in that you can't have an empty table cell.

I don't know. That seems like a different situation, in that an empty table cell feels appropriate. That signifies "No data point" rather than "No content".

I can't think of a case where we'd have a model without children content. Thats's not to say its not a valid case, but I think we can move forward with this as-is. If we come across the use-case, we'll re-evaluate.

But if a developer passes in null or no children, does it change the display logic of the component? If the dialog can't display if this is null or empty, I'm fine leaving it as required. However, it the dialog handles fine with null or empty children, I still vote to drop the isRequired off this prop. It doesn't change our implementation logic, but still allows for our API to handle that edge case.

This conversation is dumb and not important. I have removed the required flag because the impact in either direction is minimal and not worth the effort already invested in the discussion.

74c002d

JakeLaCombe · 2018-02-05T21:17:49Z

packages/terra-dialog/src/Dialog.jsx

 };

-const Dialog = ({ header, footer, children }) => {
+const defaultProps = {
+  header: null,


Anything required doesn't need a defaultProp I believe.

JakeLaCombe · 2018-02-05T21:18:41Z

packages/terra-dialog/src/Dialog.scss

@@ -3,4 +3,19 @@
    height: 100%;
    width: 100%;
  }
+
+  .header { // May not be needed if action header is already themeable.


In my opinion, I would still theme it. Terra-time-input had to apply individual styles to all of it's buttons and inputs so that it would all be aligned with each other.

JakeLaCombe · 2018-02-05T21:19:28Z

packages/terra-dialog/src/Dialog.scss

+  }
+
+  .dialog-body {
+    background-color: var(--terra-dialog-body-background-color, transparent);


How about the foreground? Typically, when you set a background, you also need to set a foreground. Or will that be handled by another component?

JakeLaCombe · 2018-02-05T21:20:09Z

packages/terra-dialog/src/DialogBody.jsx

+  ]);
+
+  return (
+    <div className={DialogBodyClassNames}>


we should probably still provide the support for custom props on this component.

JakeLaCombe · 2018-02-05T21:31:43Z

packages/terra-site/src/examples/dialog/DialogDefault.jsx

 import React from 'react';
 import Dialog from 'terra-dialog';

-const header = <span>Header Stuff</span>;
+const header = 'Header Stuff';


If it's possible, it would be better if we allowed this to take in react nodes as well. With react nodes, you can build components that easily let you opt in and out of overflowing text

Can't. This gets passed into ActionHeader, which requires String.

JakeLaCombe · 2018-02-05T21:32:40Z

packages/terra-site/src/examples/dialog/DialogDefault.jsx

+const body = (<Dialog.Body>
+  <p>This is my body content.</p>
+  <p>This is some more content.</p>
+</Dialog.Body>);

 const DialogDefault = () => (
  <div style={{ height: '200px', width: '350px', border: 'dashed' }}>


Given how large this hash is, I think it's best to extract it to a CSS file. Also, should the border be themeable? @neilpfeiffer

Disregard. This is an example page.

JakeLaCombe · 2018-02-05T22:41:41Z

packages/terra-dialog/package.json

@@ -32,6 +32,7 @@
    "classnames": "^2.2.5",
    "prop-types": "^15.5.8",
    "terra-base": "^2.0.0",
+    "terra-clinical-action-header": "^1.6.0",


Talked with @mjhenkes on this some, and we believe that it might be ideal to move action header to the core repository. terra-core was the building blocks for repos such as terra-clinical and terra-consumer, so it feels like a circular dependency when terra-core relies on stuff set inside one of the repos that extend off of this one.

Talked in Scrum. Current decision is to move forward with this review and add themes to the component in Clinical. Later, we are going to evaluate deprecating the clinical ActionHeader and including a new one in core.

StephenEsser · 2018-02-06T17:39:08Z

packages/terra-dialog/src/DialogBody.jsx

+};
+
+const DialogBody = ({ children }) => {
+  const DialogBodyClassNames = cx([


This variable should probably be camelCase

2fabef8

EDIT: The was actually a file I missed. Running tests now, but will update with proper commit in a bit.

EDIT 2: 92cd4ef

…com/cerner/terra-core into CWELL-15317-theme-and-action-header

bjankord · 2018-02-09T02:00:21Z

packages/terra-modal/src/Modal.scss

-    background-color: #fff;
-    border-radius: 5px;
+    background-color: var(--terra-modal-background-color, #fff);
+    border: var(--terra-modal-border, none);


If there is no property value for default theme, lets drop the fallback value for these theming properties.

e.g. switch border: var(--terra-modal-border, none); to border: var(--terra-modal-border);

Looks like this can be done for the following

border

box-shadow

color

height

margin

max-width

width

…com/cerner/terra-core into CWELL-15317-theme-and-action-header

JakeLaCombe

Looks good. My only last request is to see if we can reduce the size of those screenshots possibly. toMatchScreenshot can take a screenshot for a given selector, which we could probably get from the dialog wrapper.

mmalaker · 2018-02-09T22:03:13Z

Functional verification completed. The dialog examples display and function correctly with the action header changes applied.

neilpfeiffer · 2018-02-12T13:40:31Z

packages/terra-dialog/src/Dialog.scss

+  .dialog-header {
+    background-color: var(--terra-dialog-header-background-color, transparent);
+    border-bottom: var(--terra-dialog-header-bottom-border, none);
+    color: var(--terra-dialog-header-foreground-color, #000);


@saedar - Minor color updates:

.dialog-header { color: var(--terra-dialog-header-foreground-color, #1c1f21); } .dialog-body { color: var(--terra-dialog-body-foreground-color, #1c1f21); } .dialog-footer { border-top: var(--terra-dialog-footer-bottom-top, 1px solid #dedfe0); color: var(--terra-dialog-footer-foreground-color, #1c1f21); }

neilpfeiffer · 2018-02-12T17:05:36Z

Tested: PR #1209
Resolves: Issue #1157 #416

Reviewed:

Follows pattern considerations documented in Cerner Design Standards: Dialogs
Follows UX defined design implementation for Terra-Dialog
Follows defined theme designs: terra-theme

Future Enhancments:

Action header will be uplifted to universal design in the future
Action footer will be created with universal design in the future

Verified by @scottwilmarth and @neilpfeiffer, Ready for Release after color updates.

Pass
UX Review

Perrin,Ryan added 7 commits January 31, 2018 12:13

Refactored component to provide subcomponents. Added theme variables.…

7169209

… Added new example. Added action header.

Removed some testing code.

0fb172a

Added thememing to Modal and ModalOverlay.

5e21a2f

Updated theme variable defaults.

d9342b7

fixed lint

ade580d

Updated docs. Updated tests.

09cd6f7

Updated tests. Updated doc. Updated CHANGELOGs.

44bcf73

saedar added the 📦 terra-dialog label Feb 5, 2018

saedar self-assigned this Feb 5, 2018

bjankord temporarily deployed to terra-core-deployed-pr-1209 February 5, 2018 20:01 Inactive

saedar commented Feb 5, 2018

View reviewed changes

Merge branch 'master' into CWELL-15317-theme-and-action-header

cb8d64f

bjankord temporarily deployed to terra-core-deployed-pr-1209 February 5, 2018 20:06 Inactive

JakeLaCombe reviewed Feb 5, 2018

View reviewed changes

StephenEsser reviewed Feb 6, 2018

View reviewed changes

Perrin,Ryan added 2 commits February 6, 2018 14:16

Removed old doc. Added theme vars for foreground. Fixed minor typos.

2fabef8

Merge branch 'CWELL-15317-theme-and-action-header' of https://github.…

99ebff7

…com/cerner/terra-core into CWELL-15317-theme-and-action-header

bjankord temporarily deployed to terra-core-deployed-pr-1209 February 6, 2018 20:21 Inactive

Fixed lint.

392409f

bjankord temporarily deployed to terra-core-deployed-pr-1209 February 7, 2018 15:24 Inactive

bjankord temporarily deployed to terra-core-deployed-pr-1209 February 8, 2018 18:08 Inactive

Merge branch 'master' into CWELL-15317-theme-and-action-header

11a47e2

bjankord temporarily deployed to terra-core-deployed-pr-1209 February 8, 2018 20:05 Inactive

bjankord reviewed Feb 9, 2018

View reviewed changes

Perrin,Ryan added 2 commits February 9, 2018 09:02

Removed CSS defaults.

5385d5a

Merge branch 'CWELL-15317-theme-and-action-header' of https://github.…

13641e6

…com/cerner/terra-core into CWELL-15317-theme-and-action-header

bjankord temporarily deployed to terra-core-deployed-pr-1209 February 9, 2018 15:04 Inactive

JakeLaCombe approved these changes Feb 9, 2018

View reviewed changes

bjankord approved these changes Feb 9, 2018

View reviewed changes

bjankord added UX Review Required Functional Review Required labels Feb 9, 2018

tbiethman approved these changes Feb 9, 2018

View reviewed changes

Updated screenshots.

63f4d6c

bjankord temporarily deployed to terra-core-deployed-pr-1209 February 9, 2018 21:06 Inactive

Merge branch 'master' into CWELL-15317-theme-and-action-header

8baa2bb

bjankord temporarily deployed to terra-core-deployed-pr-1209 February 9, 2018 21:10 Inactive

mmalaker removed the Functional Review Required label Feb 9, 2018

Merge branch 'master' into CWELL-15317-theme-and-action-header

ed8cd4c

bjankord temporarily deployed to terra-core-deployed-pr-1209 February 9, 2018 23:10 Inactive

neilpfeiffer reviewed Feb 12, 2018

View reviewed changes

Adjusted variable defaults from UX feedback.

d2d0eb7

bjankord temporarily deployed to terra-core-deployed-pr-1209 February 12, 2018 17:31 Inactive

Merge branch 'master' into CWELL-15317-theme-and-action-header

53f2a7e

bjankord deployed to terra-core-deployed-pr-1209 February 12, 2018 17:33 View deployment

neilpfeiffer removed the UX Review Required label Feb 12, 2018

saedar merged commit 82a217e into master Feb 12, 2018

saedar deleted the CWELL-15317-theme-and-action-header branch February 12, 2018 18:16

tbiethman mentioned this pull request Apr 2, 2018

[terra-modal-manager] Increase specificity of ModalManager size selectors cerner/terra-framework#134

Closed

4 tasks

		```

		Note: This component has an i18n dependency that requires setup per [these steps](https://github.com/cerner/terra-core/#internationalization-i18n).

Dialog - Add Theme and ActionHeader #1209

Dialog - Add Theme and ActionHeader #1209

Conversation

saedar commented Feb 5, 2018 • edited by neilpfeiffer Loading

Summary

Deployment URL

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

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

saedar Feb 6, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

JakeLaCombe left a comment

Choose a reason for hiding this comment

mmalaker commented Feb 9, 2018

neilpfeiffer Feb 12, 2018 • edited Loading

Choose a reason for hiding this comment

neilpfeiffer commented Feb 12, 2018

saedar commented Feb 5, 2018 •

edited by neilpfeiffer

Loading

saedar Feb 6, 2018 •

edited

Loading

neilpfeiffer Feb 12, 2018 •

edited

Loading