[Subheader] new Component (Recover 2979) #3033

alitaheri · 2016-01-24T20:15:05Z

This is the recovery of rebase-gone-wrong from #2979.

@pradel Is this all of it? Did I miss something? if so, I'll look through the commits again. Just make sure it's all there 😁

@oliviertassinari @newoga @mbrookes Continue reviewing here :D

oliviertassinari · 2016-01-24T20:16:35Z

src/Subheader/Subheader.jsx

+  inset: false,
+};
+
+let Subheader = ({children, muiTheme, inset, style, ...other}) => {


I would use a props variable here. That will be better for adding new property.

@oliviertassinari ok, but let's agree on one style for single arguments in lambdas (props) => or props =>?

That's a tough question 😁. I would say (props) => for when we add a new parameter in the future.

Ok then it's settled 😆 😆

alitaheri · 2016-01-24T20:20:57Z

Should I add a documentation page for this too? ( I'm in for adding a page rather than random mentions on other components)

Maybe a new section where this and divider and it's kind could go under?

Something like: Miscellaneous, or Utility, or... I don't know! naming is NP-Complete in my book :|

alitaheri · 2016-01-24T20:22:01Z

@pradel Sorry, I'm intercepting your PR. this was the best way I could get it back up and running so we can have it in the next release.

oliviertassinari · 2016-01-24T20:22:03Z

src/grid-list/grid-list.jsx

@@ -101,6 +101,7 @@ const GridList = React.createClass({
    const mergedRootStyles = this.mergeStyles(styles.root, style);

    const wrappedChildren = React.Children.map(children, (currentChild) => {
+      if (currentChild.type.displayName === 'Subheader') return currentChild;


currentChild and currentChild.type can be null or undefined.
What about using React.isValidElement like in TableRow?
Or

if (!currentChild) { return null; } if (!currentChild.type) { return currentChild; }

like in ToolbarGroup?

React.isValidElement seems more convenient, no?

Alright, the implementation looks good: https://github.com/facebook/react/blob/master/src/isomorphic/classic/element/ReactElement.js#L283.

newoga · 2016-01-24T20:47:43Z

@alitaheri I guess one option is just import examples from List and Grid (and eventually Menu) that use Subheader and write a different description name for it? I don't know if it's better to rewrite new and possibly redundant examples

alitaheri · 2016-01-24T20:51:30Z

@newoga That's one possibility, I like it 😁 @oliviertassinari What do you think.

Also, how about putting this and Divider under some section with some name? since these are general and are mostly used by our own components. Something like Miscellaneous, or Utility, etc.

alitaheri · 2016-01-24T20:59:02Z

Build fails because of jsx-eslint/eslint-plugin-react#408.

I'm investigating.

oliviertassinari · 2016-01-24T21:47:47Z

I don't know if it's better to rewrite new and possibly redundant examples

Using examples in two different page sounds like a good idea.

Also, how about putting this and Divider under some section with some name?

I'm not sure, what would be the benefit? If it's for reducing the length of the LeftNav list. We can merge the DatePicker and TimePicker under a Pickers section. Same for the TextField SelectField Toggle Checkbox RadioButton, we could merge them under a Form section.

alitaheri · 2016-01-24T21:59:58Z

I'm not sure, what would be the benefit?

Well the length doesn't matter, besides the flatter they are the easier it is to find components.

DatePicker and TimePicker under a Pickers

That would make it hard to find these since Picker has neither Time not Date 😁

I'm just thinking out load. I don't see any benefit myself. And if it's ok with all, I'll just make a Subheader Page :P

oliviertassinari · 2016-01-24T22:01:57Z

That would make it hard to find these since Picker has neither Time not Date

But, this is what Google is doing. Are you criticizing their website 😁 ? https://www.google.com/design/spec/components/pickers.html
(I'm not sure what is best here)

alitaheri · 2016-01-24T22:06:39Z

@oliviertassinari Yes actually 😆 😆 Because it took me some time to find the TimePicker there when I first visited their website 😆 😆

newoga · 2016-01-24T22:12:22Z

@alitaheri @oliviertassinari

I'll let you two fight and agree with whoever is still alive 😨 😆

I'm fine with the Subheader page, I think it might make sense to generally follow the spec layout. We may regret it one day but it's worth trying haha.

alitaheri · 2016-01-24T22:14:31Z

I'll let you two fight and agree with whoever is still alive 😨 😆

😆 😆 😆

I'm fine with the Subheader page

Alright then I'll work on it tomorrow. it's 1:44 AM here. 😪 😪

pradel · 2016-01-24T22:47:29Z

@alitaheri Thanks a lot it's all there :)

mbrookes · 2016-01-24T23:40:16Z

I like the idea of grouping the form components together - 'switches' by itself always felt unnatural to me, but in that case, the "pickers" are also form components. That doesn't solve the problem of where to put utility components however. (I'd throw Paper and Popover into that bucket.)

alitaheri · 2016-01-25T06:30:38Z

@pradel Glad to hear it 😁

@mbrookes Well I'll just add it in the root, let them pile up a bit more (we wanna add Tooltip to the mix, and more) and then we'll figure out the best categorization. for now I think we don't have enough components that we should organize :P

mbrookes · 2016-01-25T12:29:54Z

@alitaheri Arguably also Badge, so that's four even without Tooltip... is that enough? 😁

alitaheri · 2016-01-25T18:49:50Z

Guys take another look at this.

@pradel I hope this is how you hoped it would turn out 😁

@oliviertassinari @newoga Take a look at the docs. I don't know it the titles are right.

@mbrookes Let's discuss categorization in it's own issue. It's very debatable 😆 I personally think Badge is a first class component 😁

P.S. I had to implement a way to be able to ignore certain props. see if it's all good. I think @ignore is more expressive than @internal, we might want to add @internal for something else yet display it in the table. But no one wants to display @ignored props 😁

Almost forgot. I also deprecated subheader props of list 😆

newoga · 2016-01-25T20:33:53Z

docs/src/app/components/pages/components/List/Page.jsx

@@ -28,7 +28,7 @@ import ListExampleSelectable from './ExampleSelectable.jsx';

 const descriptions = {
  simple: 'A simple `List` with left and right [SVG icons](/#/components/svg-icon).',
-  chat: 'A chat list with Image [Avatars](/#/components/avatar) and List `subheaders`.',
+  chat: 'A chat list with Image [Avatars](/#/components/avatar) and `Subheader`.',


I supposed we might as well link to the subheader docs since we're doing it with Avatars here. Thoughts?

Yeah, good idea 👍 👍

newoga · 2016-01-25T20:39:42Z

I think i'm good with this other than the few debatable comments I made.

Almost forgot. I also deprecated subheader props of list 😆

I like deprecating and simplifying, but should we just add this the the list of things we should deprecate in 0.15.0? I'm worried if the subheader prop is used a lot and might annoy people for a patch release.

(I'm all for documenting the new way of doing it though)

alitaheri · 2016-01-25T20:48:12Z

@newoga I'm not going to merge this before the 0.14.3 release. For the exact same concern 😁

newoga · 2016-01-25T20:48:35Z

Sounds good!

mbrookes · 2016-01-26T21:09:04Z

docs/src/app/components/pages/components/GridList/Page.jsx

@@ -12,7 +12,7 @@ import gridListExampleComplexCode from '!raw!./ExampleComplex';
 import GridListExampleComplex from './ExampleComplex';

 const descriptions = {
-  simple: 'A simple example of a scrollable `GridList`.',
+  simple: 'A simple example of a scrollable `GridList` containing a `Subheader`.',


Should that perhaps link to the Subheader docs page?

Yes, we discussed it for some other places too. I'll provide links in my next commit 😅 😅

alitaheri · 2016-01-26T21:19:13Z

Having this many collaborators is awesome!

Thank you all for your magnificent feedback 🙏 🙏

alitaheri · 2016-02-02T17:19:44Z

Sorry for all the pings 😁 @mbrookes @oliviertassinari @newoga Take a final look 😄

mbrookes · 2016-02-02T17:23:10Z

Looks good here. (Unless you wanted to add it to the change log as part of the PR...)

alitaheri · 2016-02-02T17:26:24Z

I'm usually the one who updates the change log for a release, so no difference 😁

newoga · 2016-02-02T17:26:49Z

src/lists/list.jsx

+      hasSubheader = true;
+    } else {
+      const firstChild = React.Children.toArray(children)[0];
+      if (React.isValidElement(firstChild) && firstChild.type === Subheader) {


Interesting, I didn't realize child.type === Subheader worked. Is there a reason why we're not using this patter over using displayName everywhere else in the codebase?

No idea O.o

Oh yeah, I'm really interested in removing the use of displayName for the check. Then, I would be able to write a plugin that remove the displayName from the production build 😁

Good to know...will watch out for areas we're doing this. This approach is a lot better/safer IMO.

Actually, I see one drawback here. We end up importing the <Subheader /> component even if we don't need it.
What about using displayName 😁?

@oliviertassinari I'm gonna make an issue for this kind of things so we can fully discuss what we should do 👍 Thanks for pointing it out

newoga · 2016-02-02T17:27:24Z

@alitaheri This looks good to me too! 👍

mbrookes · 2016-02-02T17:28:25Z

I know - but saw you added a HEAD section...

newoga · 2016-02-02T17:30:36Z

There are few styling questions we may have to revisit with this component in the future, like how much padding we should use when it's above an avatar, and if we need to extend support for <Menu /> but I think this is good for now.

alitaheri · 2016-02-02T17:36:01Z

I know - but saw you added a HEAD section...

Yeah, actually I proposed a note to be added somewhere to ask contributers to add a line there. but the I realized it's harder this way with a lot of overhead (how to write it, what to add what not to add, etc...) So I decided it's just easier to build it up from the git log ( github helps a lot). So I don't bother doing it my self either. since I'll do it all at once with more concentration.

@newoga Yeah, I think you're right. might not be a bad idea to continue that in another issue. 😁

mbrookes · 2016-02-02T17:38:37Z

@alitaheri makes sense.

alitaheri · 2016-02-02T20:13:03Z

Let's not merge this just yet 😁 we should wait for the 0.14.4 release.

alitaheri · 2016-02-03T17:43:36Z

@oliviertassinari It's done. feel free to merge if all's good. thats 😁

[Subheader] new Component (Recover 2979)

oliviertassinari · 2016-02-03T17:47:55Z

@pradel @alitaheri Thanks, that's a great addition to the lib 👍!

alitaheri · 2016-02-03T17:49:41Z

Thanks a lot @pradel This was a great step toward composable components 👍 🎉

newoga · 2016-02-03T18:05:00Z

🎉 Thanks @pradel! Thanks @alitaheri for picking this up 😄

alitaheri added the PR: Needs Review label Jan 24, 2016

oliviertassinari reviewed Jan 24, 2016
View reviewed changes

alitaheri force-pushed the recover-2979 branch from 5516b68 to 05c04d1 Compare January 25, 2016 18:47

newoga reviewed Jan 25, 2016
View reviewed changes

newoga mentioned this pull request Jan 26, 2016

[Divider] Fixes code formatting conventions for Divider component #3049

Merged

mbrookes reviewed Jan 26, 2016
View reviewed changes

newoga reviewed Feb 2, 2016
View reviewed changes

pradel and others added 4 commits February 3, 2016 21:05

Add Subheader component

9d51dfd

Deprecate subheader from List

0b758ba

Add Subheader page to the docs

e86ea3d

Implement the ability to ignore certain props

cd6c96d

alitaheri force-pushed the recover-2979 branch from c199839 to cd6c96d Compare February 3, 2016 17:37

mbrookes added PR: needs revision The pull request can't be merged. More details is available on the code review or fails in the CI and removed PR: Needs Review labels Feb 3, 2016

mbrookes added PR: Needs Review and removed PR: needs revision The pull request can't be merged. More details is available on the code review or fails in the CI labels Feb 3, 2016

oliviertassinari added PR: Review Accepted and removed PR: Needs Review labels Feb 3, 2016

oliviertassinari added a commit that referenced this pull request Feb 3, 2016

Merge pull request #3033 from alitaheri/recover-2979

13699db

[Subheader] new Component (Recover 2979)

oliviertassinari merged commit 13699db into mui:master Feb 3, 2016

alitaheri deleted the recover-2979 branch February 3, 2016 17:49

alitaheri mentioned this pull request Feb 3, 2016

[Core] Checking the type of elements for specialized behavior #3163

Closed

zannager added the package: styles Specific to @mui/styles. Legacy package, @material-ui/styled-engine is taking over in v5. label Mar 15, 2023

[Subheader] new Component (Recover 2979) #3033

[Subheader] new Component (Recover 2979) #3033

Conversation

alitaheri commented Jan 24, 2016

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

alitaheri commented Jan 24, 2016

alitaheri commented Jan 24, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

newoga commented Jan 24, 2016

alitaheri commented Jan 24, 2016

alitaheri commented Jan 24, 2016

oliviertassinari commented Jan 24, 2016

alitaheri commented Jan 24, 2016

oliviertassinari commented Jan 24, 2016

alitaheri commented Jan 24, 2016

newoga commented Jan 24, 2016

alitaheri commented Jan 24, 2016

pradel commented Jan 24, 2016

mbrookes commented Jan 24, 2016

alitaheri commented Jan 25, 2016

mbrookes commented Jan 25, 2016

alitaheri commented Jan 25, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

newoga commented Jan 25, 2016

alitaheri commented Jan 25, 2016

newoga commented Jan 25, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

alitaheri commented Jan 26, 2016

alitaheri commented Feb 2, 2016

mbrookes commented Feb 2, 2016

alitaheri commented Feb 2, 2016

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

newoga commented Feb 2, 2016

mbrookes commented Feb 2, 2016

newoga commented Feb 2, 2016

alitaheri commented Feb 2, 2016

mbrookes commented Feb 2, 2016

alitaheri commented Feb 2, 2016

alitaheri commented Feb 3, 2016

oliviertassinari commented Feb 3, 2016

alitaheri commented Feb 3, 2016

newoga commented Feb 3, 2016