[Doc] Convert Dialog to use the new standard. #2483

alitaheri · 2015-12-11T16:46:38Z

oliviertassinari · 2015-12-11T16:54:33Z

docs/src/app/components/Dialog/README.md

@@ -0,0 +1,6 @@
+## Dialog
+
+Dialog can only be a controlled component.


Could you add a link to https://www.google.com/design/spec/components/dialogs.html?

oliviertassinari · 2015-12-11T16:58:18Z

@subjectix Wow, that's some hard work here!

oliviertassinari · 2015-12-11T16:59:19Z

src/dialog.jsx

+    /**
+     * The `className` to add to the `Dialog` window content container.
+     * This is the `Paper` element that is seen when the `Dialog` is shown.
+     */


Should we add the className property in the doc?

yeah, but who will take the className? I wanted to ask, I forgot 😅

I think we have to make an standard.

for each xxxStyle we should have xxxClassName and for each style a className, don't you think?

alitaheri · 2015-12-11T17:06:58Z

Wow, that's some hard work here!

Thanks ^^

alitaheri · 2015-12-11T17:08:02Z

I'll take a look at the test

alitaheri · 2015-12-11T18:29:49Z

@oliviertassinari I fixed the test and added a whole bunch of props for completeness.

This is only for demonstration, please take a long good look at my PR 👯

Do you think this set of (standardized) props make the library a lot easier to customize? or it only bloats the components?

alitaheri · 2015-12-11T18:34:00Z

@newoga I'd like to hear your idea on this one too, if you don't mind ^^

newoga · 2015-12-11T22:26:43Z

Do you think this set of (standardized) props make the library a lot easier to customize? or it only bloats the components?

@subjectix, this is a very interesting problem and something I've been considering lately. Below are some of my initial thoughts:

My first thought is, either way, I like standardization and API predictability. The consistent {name}Style {name}ClassName makes the API more predictable which I like, (ie if you learn that pattern for one component, you'll get the idea for all components). I also think it would be cool if inline-styles weren't the only way to extend and customize component style.

Some possible problems in the current implementation that come to mind that we might have to look into/investigate further are:

1. Style precedence may limit effectiveness of {name}ClassName props for customizability

Based on my understanding, right now for most of our components, when users provide their own {name}Style prop, we merge their style object into our default/internal style definition letting them override pretty much anything (for better or worse).

Since inline styles always take precedence over classes, will our default/internal style definitions ultimately override css properties that matter the most?

Simple example would be if someone attempted to do this:

.dialog-title {
  color: 'orange';
}

<Dialog {...other} titleClassName="dialog-title" />

Unless we'd have the ability to export our internal inline-styles to stylesheets and have them loaded before theirs, I think their css class would be ineffective in this particular use case.

2. Possible increased API surface area for components that have several layers of deeply nested children

I think you suggested that this might be a concern. I don't think any of our current components excessively suffer from this right now, but I think it's valid concern nonetheless.

Since I recently worked with the text-field.jsx component, I started wondering if we might eventually run into issues if the props on our parent component are too coupled with our children components. We kind of discussed some of the challenges with the whole errorText vs helperText in #2474.

Other random thoughts:
It's possible that the problem right now with customizability is that consumers of material-ui don't have enough flexibility in modifying how styles are applied. Not because they don't have enough flexibility in where styles are applied.

I'd be interested in exploring programatic approaches for developers to hook into the style transformation for material-ui components, or approaches that allow consumers to essentially build material components during build time or run time based on their own definitions.

It might mean that components like TextField are really high order components themselves that can take custom style transformations that process CSS, inline-styles, or whatever it be, and returns a <TextField /> component that works and looks the way they want it to. They could put those components in their own projects in their own material folders and import them themselves so they don't have to reapply customizations each time they use it.

I know it's really abstract right now. I haven't tried anything but I think it's certainly possible. Part of me wonders if something like this will be a necessity to even support desktop on native/mobile at the same time.

alitaheri · 2015-12-11T23:17:46Z

@newoga

I think their css class would be ineffective in this particular use case.

Well it's a common pattern for those who want to override react inline styles with css classes to use the !important modifier. So it is practical, and there are some issues submitted wanting css classes hooks so they can use this capability of css (goo thing we have it 😅 )

Possible increased API surface

I think this is a valid trade-off, maybe if we can categorize props and make separate tables for functionality and customization the surface would become cleaner?

But maybe, if the users see xxxClassName and xxxStyle all over the place, they'll easily get used to it. a limiting API is much worse than an over populated one, imo.

@oliviertassinari Thoughts?

newoga · 2015-12-11T23:25:15Z

But maybe, if the users see xxxClassName and xxxStyle all over the place, they'll easily get used to it. a limiting API is much worse than an over populated one, imo.

I definitely agree with this. If I'm honest with myself, I think it feels more natural for components to be able to accept both classes and styles so they feel more normal.

I think this is a valid trade-off, maybe if we can categorize props and make separate tables for functionality and customization the surface would become cleaner?
That's true, there are probably definitely approaches to organization documentation so that it is clearer and more approachable. Also, now that we have doc gen, its a ton easier to maintain.

From a pure maintenance standpoint, having className doesn't add a lot of risk/work because we're simply going to pass it directly to the relevant component.

alitaheri · 2015-12-11T23:27:37Z

@newoga Well the issue isn't maintenance actually, I just want to follow the best convention and make it standard from now on. Need to have people agreeing with this approach. This is not a small decision :D

alitaheri · 2015-12-13T23:13:14Z

@oliviertassinari Take a final look at this when you get the time ^_^

oliviertassinari · 2015-12-13T23:24:02Z

src/dialog.jsx

+    children: React.PropTypes.node,
+
+    /**
+     * The `className` to add to the `Dialog` root element.


What about The css class name of the root element.?

oliviertassinari · 2015-12-13T23:30:47Z

Could you update the example of the #/customization/themes page? I can see some warnings:

Warning: Failed propType: Invalid prop `actions` supplied to `Dialog`, expected a ReactNode. Check the render method of `ThemesPage`.

oliviertassinari · 2015-12-13T23:32:00Z

src/dialog.jsx

@@ -116,20 +117,26 @@ const DialogInline = React.createClass({

  propTypes: {
    actionFocus: React.PropTypes.string,
-    actions: React.PropTypes.array,
+    actions: React.PropTypes.node,
+    actionsContainerClassName:  React.PropTypes.string,


Double spaces here. The rule relative eslint rule is not inforced yet.

ok, thanks.

oliviertassinari · 2015-12-13T23:36:07Z

@subjectix I have done some comment on the code. Otherwise, that a great improvement for this component 👍.

alitaheri · 2015-12-13T23:44:53Z

@oliviertassinari Please take another look, I remove Dialog from some other comments too.

oliviertassinari · 2015-12-14T09:45:11Z

What about renaming Dialog/ModalExample.jsx to Dialog/ExampleModal.jsx to follow the component name DialogExampleModal, same for the other example?
Otherwise, I'm Ok to merge this 😄.

alitaheri · 2015-12-14T10:07:50Z

@oliviertassinari Ok, I'll change the names, squash and merge, thanks for your feedbacks 👍 😄

[Doc] Convert Dialog to use the new standard.

oliviertassinari · 2015-12-14T11:54:32Z

🎉

alitaheri added docs Improvements or additions to the documentation PR: Needs Review labels Dec 11, 2015

alitaheri added this to the Improve the documentation milestone Dec 11, 2015

oliviertassinari mentioned this pull request Dec 11, 2015

Migration of the documentation #2433

Closed

29 tasks

oliviertassinari reviewed Dec 11, 2015
View reviewed changes

alitaheri mentioned this pull request Dec 11, 2015

Deprecate usage of JSON in favor of composability. #2456

Closed

4 tasks

oliviertassinari reviewed Dec 13, 2015
View reviewed changes

dialog: migrate docs, deprecate actionJSON, improve customization

4074a1f

alitaheri force-pushed the doc-codegen-dialog branch from a9f7106 to 4074a1f Compare December 14, 2015 10:35

alitaheri added PR: Review Accepted and removed PR: Needs Review labels Dec 14, 2015

alitaheri added a commit that referenced this pull request Dec 14, 2015

Merge pull request #2483 from subjectix/doc-codegen-dialog

37f71b6

[Doc] Convert Dialog to use the new standard.

alitaheri merged commit 37f71b6 into mui:master Dec 14, 2015

alitaheri deleted the doc-codegen-dialog branch December 14, 2015 11:05

oliviertassinari mentioned this pull request Dec 14, 2015

Add support for id attributes in dialog actions (https://github.com/c… #1655

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Doc] Convert Dialog to use the new standard. #2483

[Doc] Convert Dialog to use the new standard. #2483

alitaheri commented Dec 11, 2015

oliviertassinari Dec 11, 2015

alitaheri Dec 11, 2015

oliviertassinari commented Dec 11, 2015

oliviertassinari Dec 11, 2015

alitaheri Dec 11, 2015

alitaheri Dec 11, 2015

alitaheri commented Dec 11, 2015

alitaheri commented Dec 11, 2015

alitaheri commented Dec 11, 2015

alitaheri commented Dec 11, 2015

newoga commented Dec 11, 2015

alitaheri commented Dec 11, 2015

newoga commented Dec 11, 2015

alitaheri commented Dec 11, 2015

alitaheri commented Dec 13, 2015

oliviertassinari Dec 13, 2015

oliviertassinari commented Dec 13, 2015

oliviertassinari Dec 13, 2015

alitaheri Dec 13, 2015

oliviertassinari commented Dec 13, 2015

alitaheri commented Dec 13, 2015

oliviertassinari commented Dec 14, 2015

alitaheri commented Dec 14, 2015

oliviertassinari commented Dec 14, 2015

		@@ -0,0 +1,6 @@
		## Dialog

		Dialog can only be a controlled component.

[Doc] Convert Dialog to use the new standard. #2483

[Doc] Convert Dialog to use the new standard. #2483

Conversation

alitaheri commented Dec 11, 2015

Choose a reason for hiding this comment

Choose a reason for hiding this comment

oliviertassinari commented Dec 11, 2015

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

alitaheri commented Dec 11, 2015

alitaheri commented Dec 11, 2015

alitaheri commented Dec 11, 2015

alitaheri commented Dec 11, 2015

newoga commented Dec 11, 2015

alitaheri commented Dec 11, 2015

newoga commented Dec 11, 2015

alitaheri commented Dec 11, 2015

alitaheri commented Dec 13, 2015

Choose a reason for hiding this comment

oliviertassinari commented Dec 13, 2015

Choose a reason for hiding this comment

Choose a reason for hiding this comment

oliviertassinari commented Dec 13, 2015

alitaheri commented Dec 13, 2015

oliviertassinari commented Dec 14, 2015

alitaheri commented Dec 14, 2015

oliviertassinari commented Dec 14, 2015