UiFramework: Modals in react #11500

stacey-gammon · 2017-04-28T18:22:42Z

Add modals in the ui framework. Not used yet in angular.

w33ble · 2017-05-01T20:34:59Z

ui_framework/components/index.js

@@ -4,9 +4,15 @@ export {
  KuiButtonIcon,
  KuiLinkButton,
  KuiSubmitButton,
-} from './button';
+} from './button/index';


Is there a reason you explicitly have to include index here? This doesn't seem to be related to this PR at all...

w33ble · 2017-05-01T20:38:49Z

src/ui/public/react_components.js

 const app = uiModules.get('app/kibana', ['react']);
 app.directive('toolBarSearchBox', function (reactDirective) {
  return reactDirective(KuiToolBarSearchBox);
 });
+app.directive('confirmModal', function (reactDirective) {


Are there any future plans for removing this and being able to use React components like we would use Angular components? Or, I'm assuming this gets auto-loaded somewhere in the Angular bootup, so is the plan to no longer import the components we need wherever we need them? I'm a bit out of touch with our "React in Angular" plans...

It gets imported in public/kibana, so anyone can use them. For ui_framework components, which are generic, it's nice to include them once so you can use them everywhere. For react components that are specific to a certain area, then I would import them where you need them.

So no, no plans to remove this.

w33ble · 2017-05-01T20:40:04Z

ui_framework/components/index.js

+export {
+  KuiConfirmModal,
+  KuiModalOverlay
+} from './modal/index';


Here as well, why do you need to include the index?

w33ble · 2017-05-01T20:48:55Z

ui_framework/components/modal/confirm_modal.js

+}
+
+KuiConfirmModal.propTypes = {
+  message: React.PropTypes.string,


Note: React.PropTypes is deprecated as of React v15.5. Please use the prop-types library instead.

React.PropTypes has been depricated, so we shouldn't be using them in new code. Use the prop-types module instead.

w33ble · 2017-05-01T21:05:40Z

ui_framework/components/modal/modal.js

+
+export function KuiModal({ children, ...rest }) {
+  return (
+    <div className="kuiModal" { ...rest }>


I really dislike this "rest" pattern. I much prefer to be explicit about the arguments you pass along, or possibly pull them off context or something (though I'll admit I don't even know if that's a thing, I haven't ever used context before...).

But, that conversation aside, doing this means that you can clobber the className on components. Here's an example. Is this intentional? Do you want the className to be overridden?

I agree with you @w33ble, but we've had this discussion in a previous PR for React buttons. Here's some context:

Original comment: [UI Framework] Create Button React components in UI Framework. #10646 (comment)

Discussion: [UI Framework] Create Button React components in UI Framework. #10646 (comment)

That being said, you're right about the problem with clobbering the className. We should be concatenating these strings, not overriding them:

const { className, children, ...rest } = props const classes = classnames('my-class', className) return <a className={ classes } { ...rest }>{ children }</a>

I mostly dislike the "rest" pattern unless it's for low-level components that are basically just "simple DOM wrappers" where we usually want to push multiple attributes through (like a wrapper around a, where we don't actually do anything to href etc in the component).

(Though, I've never built components in this way where they are all just "one-line wrappers all the way down", so I don't have any experience in this context to base my opinions on)

Pulled out className to avoid overwriting them.

@kjbekkelund... so you are saying use ...rest for this component because it's a simple wrapper, but for the likes of KuiConfirmModal, which is more complicated, manually pass them in?

I'm fine with whatever.

What I think would be nice is a helper function to build these components, where all they are doing is using a className with children. There are so many of them that look identical except for a class name. But I hear a lot of feedback that I tend to over-DRY so I'll leave that alone for now. :)

FYI, these required html props, as far as I can tell, have to be handled somewhat specially because of the - in their name. So instead of doing:

KuiConfirmModal({ stuff, aria-label, data-test-subj}) { ....

You have to do:

KuiConfirmModal({ stuff, ...rest }) { const ariaLabel = rest['aria-label']; const dataTestSubj = rest['data-test-subj']; ....

Unless anyone else knows a better way to handle that?

@stacey-gammon Not sure what I think of this myself, but this was my thinking: My main "concern/thought" around wrappers is that if it's a DOM wrapper you know what can be in rest (any relevant html attribute), but if it's a React component that becomes a bit more unclear (it also makes the api unclear, as it's not super-clear what you actually expect to receive and pass down into lower-level components).

Also: React will let you know if DOM elements receives the wrong attribute (https://facebook.github.io/react/warnings/unknown-prop.html).

However, I also think there are edge-cases to this. E.g. on Cloud we had a couple different Button helpers that receive rest even if they weren't "pure DOM components" (they passed on to lower-level button helpers, e.g. DangerButton wrapped SpinButton that wrapped regular DOM buttons).

I think rest is a good approach if you're expected to see it as a "DOM component" from the outside (e.g. a DangerButton that might have some extra fields, but you can give it everything you can give a normal DOM button).

Historically I've added these rest params based on actual need in the code, but that might be a bit different in this context as 1) the team is much larger, 2) we're building a UI framework.

w33ble · 2017-05-01T21:08:59Z

ui_framework/components/modal/confirm_modal.js

+import { KuiModalFooter } from './modal_footer';
+import { KuiModalHeader } from './modal_header';
+import { KuiModalBody } from './modal_body';
+import { KuiModalBodyText } from './modal_body_text';


All of these are exposed in ../modal, why not pull them all from there? Or, pull them all from ../index since they also exist in there? I don't like the root level index personally, but you're already using it below for the button...

index.js also exports this module... doesn't it create a circular dependency if you import from that module while also being exported from it?

Ah, yes, I suppose it would. One more reason for me to dislike the root level index.js that exports all the modules then. 🏆

My point about just using ../modal still stands though.

w33ble · 2017-05-01T21:20:25Z

src/ui/public/modals/confirm_modal.html

-    </button>
-  </div>
-</div>
+<confirm-modal


I assume this works because of the react_components.js file, which I'm assuming is auto loaded somewhere else. It's really odd that the src/ui/public/modals/confirm_modal.* code doesn't actually contain a reference to the React component it's using. That's a fair bit of indirection, and someone unfamiliar with the codebase probably won't know to go digging into the ui-framework code to find this, since it's not imported anywhere close to where it's being used.

Correct, and yea, it's a good point, but you have to create the angular directives that map to the react component, and I believe there can only be one. So if the react confirm modals in a couple different places, which one is going to register the directive? @weltenwort does that sound accurate?

I tried to find the original discussion for where this model came from, and it appears in the very first react icons PR (#10247), but couldn't find the discussion. Perhaps it was offline - @kjbekkelund or @cjcenizal, do you remember us discussing this?

Not sure — I'm not that knowledgeable about our Angular setup. I also agree with @w33ble (if it's not too painful to move in that direction)

yes, the angular directive is globally registered in the body of
react_component.js:

app.directive('confirmModal', function (reactDirective) { return reactDirective(KuiConfirmModal); });

Since javascript modules are singletons, it will only be executed once, no
matter how often that file is imported. If a directive of the same name is
registered again in some other place, the order of execution of the two
statements matters. The last directive definition wins.

All directives are global in an angular 1 app (as are services, filters,
etc...). I don't think there's a way around that. That's one of the things
angular 2 does better (and react, of course). The import and the usage will
always be in separate files, because you cannot import anything in an angular
template.

In that case, I think we should stick with this path, so we ensure modules from the uiFramework are only morphed into directives once.

In either case, if we want to have a longer discussion about this, I say we move it out of this specific PR since it's not something new that this PR introduced.

I say we move it out of this specific PR since it's not something new that this PR introduced

Agreed. I'm very much against auto-loading modules on startup, we used to do that, and it got very messy; explicit imports are much easier to understand. I don't know why the auto-loading stuff has come back and why there hasn't been a strong push against it. But that's not a conversation for this PR, and I'm glad to see it hasn't stopped it from progressing. I was just trying to understand our plans around the Angular+React stuff.

cjcenizal

This is awesome, just had a few requests and questions.

cjcenizal · 2017-05-01T22:04:32Z

ui_framework/components/modal/_modal.scss

@@ -4,6 +4,7 @@
  border: 1px solid $modalBorderColor;
  border-radius: $globalBorderRadius;
  box-shadow: 0 5px 22px rgba(#000000, 0.25);
+  z-index: 1001;


Can you create a variable called $globalModalDepth in the variables file and assign this value there?

@snide Just thought you'd like to see me making this kind of snideresque comment. Cherish this moment!

cjcenizal · 2017-05-01T22:06:18Z

ui_framework/components/modal/confirm_modal.js

+import { KuiModalFooter } from './modal_footer';
+import { KuiModalHeader } from './modal_header';
+import { KuiModalBody } from './modal_body';
+import { KuiModalBodyText } from './modal_body_text';


index.js also exports this module... doesn't it create a circular dependency if you import from that module while also being exported from it?

cjcenizal · 2017-05-01T22:07:59Z

src/ui/public/modals/confirm_modal.html

-  </div>
-</div>
+<confirm-modal
+  className="kui"


What does this do? I don't see this class defined anywhere.

whoops, removed

cjcenizal · 2017-05-01T22:13:05Z

ui_framework/components/modal/confirm_modal.js

+import { KuiModalBodyText } from './modal_body_text';
+import { KuiButton } from '../index';
+
+export function KuiConfirmModal({ message, title, onCancel, onConfirm, cancelButtonText, confirmButtonText, ...rest }) {


Would you mind putting each of these these variables on its own line? I find it much easier to scan when there are so many properties.

export function KuiConfirmModal({ message, title, onCancel, onConfirm, cancelButtonText, confirmButtonText, ...rest, }) {

If you're going to do that, this looks cleaner:

export function KuiConfirmModal(props) { const { message, title, onCancel, onConfirm, cancelButtonText, confirmButtonText, ...rest, } = props; //... }

I prefer @cjcenizal's approach instead of receiving props then immediately destructuring (so keeping as-is, just splitting over multiple lines).

cjcenizal · 2017-05-01T22:16:21Z

ui_framework/components/modal/confirm_modal.js

+      {
+        title ?
+          <KuiModalHeader>
+            <div className="kuiModalHeader__title" data-test-subj="confirmModalTitleText">


Can we create a KuiModalHeaderTitle component to wrap this?

Yea. Just to confirm, you are opposed to refactoring these div wrappers that have just a className? There are so many of them and creating a separate file + test for each one creates so much noise. So much copy/pasting when creating one.

cjcenizal · 2017-05-01T22:17:10Z

ui_framework/components/modal/confirm_modal.test.js

+  expect(component).toMatchSnapshot();
+
+  component.find('[data-test-subj="confirmModalConfirmButton"]').simulate('click');
+  sinon.assert.calledOnce(onConfirm);


Do we need this assertion here if we have it in the onConfirm test?

cjcenizal · 2017-05-01T22:20:05Z

ui_framework/components/modal/confirm_modal.test.js

+    { ...requiredProps }
+  />);
+  onConfirm.reset();
+  onCancel.reset();


After looking through https://facebook.github.io/jest/docs/setup-teardown.html, I think we should take advantage of the beforeEach setup function.

Can we change the spies to be let variables, and just reassign them inside of a beforeEach?

let onConfirm, onCancel; beforeEach(() => { onConfirm = sinon.spy(); onCancel = sinon.spy(); });

cjcenizal · 2017-05-01T22:25:57Z

ui_framework/components/modal/modal.js

+
+export function KuiModal({ children, ...rest }) {
+  return (
+    <div className="kuiModal" { ...rest }>


I agree with you @w33ble, but we've had this discussion in a previous PR for React buttons. Here's some context:

Original comment: [UI Framework] Create Button React components in UI Framework. #10646 (comment)

Discussion: [UI Framework] Create Button React components in UI Framework. #10646 (comment)

That being said, you're right about the problem with clobbering the className. We should be concatenating these strings, not overriding them:

const { className, children, ...rest } = props const classes = classnames('my-class', className) return <a className={ classes } { ...rest }>{ children }</a>

kimjoar · 2017-05-02T03:37:42Z

ui_framework/components/modal/__snapshots__/confirm_modal.test.js.snap

+>
+  <KuiModal
+    aria-label="aria-label"
+    className="testClass1 testClass2"


missing modal class

good catch. Fixing the className collision fixed all these.

kimjoar · 2017-05-02T03:41:28Z

ui_framework/components/modal/confirm_modal.test.js

+const onCancel = sinon.spy();
+
+test('renders KuiConfirmModal', () => {
+  const component = mount(<KuiConfirmModal


What does the snapshot look like if you shallow render this instead? (As it's built up of purely lower-level components it might make it include "less noise")

Interesting, didn't know that, yep, looks much better. switched!

pugnascotia · 2017-05-02T11:29:27Z

Just curious - was the react-overlays package considered at all?

stacey-gammon · 2017-05-02T11:54:19Z

@pugnascotia I haven't looked at that one. https://github.com/tajo/react-portal has been brough tup, but due to the react embedded in angular nature of this, I'm not sure we can take advantage of it just yet. Once we have a react component that needs to use the react modals, we'll take another look, but for right now, we can't get too fancy because of the angular side of things. At least that is my understanding of it.

w33ble

Down with React.PropTypes

w33ble · 2017-05-02T19:41:20Z

ui_framework/components/modal/modal.js

+}
+
+KuiModal.propTypes = {
+  className: React.PropTypes.string,
