Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Added GridList subheader #2979

Closed
wants to merge 25 commits into from
Closed

Added GridList subheader #2979

wants to merge 25 commits into from

Conversation

pradel
Copy link
Contributor

@pradel pradel commented Jan 19, 2016

@newoga
Copy link
Contributor

newoga commented Jan 19, 2016

Thanks for your contribution @pradel!

@oliviertassinari @alitaheri
This seems like an opportunity for composition. This subheader code is pretty much 1:1 with what's in lists/list.jsx. I'm thinking we should pull this out into a simple <Subheader /> stateless functional component and call it a day 😛 Heck, like the <Divider />, the spec even has it's own section for subheaders.

In matter of fact, this PR is an interesting discussion point because we have an opportunity here to not increase the API surface area of this component by adding additional props and making it required to just do something like:

<GridList>
  <Subheader style={customStyle}>My Subheader</Subheader>
  {otherContent}
</GridList> 

We make GridList and List code shorter, they both lose two props, and in the event someone wants a Subheader using a node rather than a string, they're not forced to shove it in a prop.

@pradel
Copy link
Contributor Author

pradel commented Jan 19, 2016

Great idea if you want I can make a PR with a new Subheadercomponent and update list also. @newoga

@newoga
Copy link
Contributor

newoga commented Jan 19, 2016

Thanks @pradel, that would be great. You can use divider.jsx as an example.

Could you put it in a Subheader folder as we've been doing for newer components like TextField and DropDownMenu? Thanks!

@pradel
Copy link
Contributor Author

pradel commented Jan 19, 2016

@newoga can you said me which solution is the best?

A List have a paddingTop of 8 without Subheader.
When you put a Subheader in a List it need to not have this padding top so what is the best way to do (sorry I am pretty new to react). Check first child in Listand if it's a Subheader put marginTop to 0 or inside Subheader check the if the parent is a List and put -8 marginTop ?

@c0b41
Copy link

c0b41 commented Jan 19, 2016

I think also appbar title need new component it would be nice

@newoga
Copy link
Contributor

newoga commented Jan 19, 2016

@pradel For now leave the <List /> props and API the same. If we removed/changed those then it would be a breaking change. For now, just remove the subheader code in <List /> without changing the props. For example, the render method in List.jsx could look something like this:

render() {
  const {
    children,
    insetSubheader,
    style,
    subheader,
    subheaderStyle,
    zDepth,
    ...other,
  } = this.props;

  const styles = {
    root: {
      padding: 0,
      paddingBottom: 8,
      paddingTop: subheader ? 0 : 8,
    },
    /** This can be removed since this would be in the Subheader.js component 
    subheader: {
      color: Typography.textLightBlack,
      fontSize: 14,
      fontWeight: Typography.fontWeightMedium,
      lineHeight: '48px',
      paddingLeft: insetSubheader ? 72 : 16,
    },*/
  };

/** This can also be removed since this would be in the Subheader.js component 
  let subheaderElement;
  if (subheader) {
    const mergedSubheaderStyles = this.mergeStyles(styles.subheader, subheaderStyle);
    subheaderElement = <div style={this.prepareStyles(mergedSubheaderStyles)}>{subheader}</div>;
  }*/

  return (
        <Paper
          {...other}
          style={this.mergeStyles(styles.root, style)}
          zDepth={zDepth}
        >
          {subheader ? <Subheader
            inset={insetSubheader}
            style={subheaderStyle}>{subheader}</Subheader> : null}
          {children}
        </Paper>
  );
}

@pradel
Copy link
Contributor Author

pradel commented Jan 20, 2016

Okay, maybe it's better to do the same for GridListto make the API consistent across components ?

@pradel
Copy link
Contributor Author

pradel commented Jan 20, 2016

@newoga this looks good to you ? I made a Subheader component, I let subheader props on both Listand GridList to make the API consistent.

@mbrookes
Copy link
Member

If the long term plan is to deprecate the subheader prop from List, it might be better to leave it out of Gridlist, rather than having two future deprecations.

@newoga
Copy link
Contributor

newoga commented Jan 20, 2016

If the long term plan is to deprecate the subheader prop from List, it might be better to leave it out of Gridlist, rather than having two future deprecations.

Yes I agree with @mbrookes here. Let's get the GridList API right the first time, we're going to be doing a round of deprecations in 0.15.0, I'll make sure to including fixing the List API (and deprecating the old API) for subheader at that time.

Otherwise, I think this looks good @pradel! If you can just make the changes to GridList I think this will be ready.

@pradel
Copy link
Contributor Author

pradel commented Jan 21, 2016

Okay so I removed the prop for GridList can you look if it's good like that ?

@newoga
Copy link
Contributor

newoga commented Jan 22, 2016

@pradel Sorry for the delayed response, can you squash your commits?

@oliviertassinari @alitaheri I think this is good for now. It's not a breaking change and helps a bit with code reusability. There are a few bigger picture things I'd like to run by your later that kind of ties into this but it's not urgent.

@alitaheri
Copy link
Member

Looks good 👍 👍 Thanks @pradel 😁

@oliviertassinari
Copy link
Member

@pradel Looks good 👍
Should we add this new component to the documentation?
I'm using a custom implementation on my project, I would love to use this new <Subheader /> component instead.

import React from 'react';
import PureRenderMixin from 'react-addons-pure-render-mixin';
import colors from 'material-ui/lib/styles/colors';

const styles = {
  root: {
    fontSize: 14,
    color: colors.lightBlack,
    margin: 16,
    fontWeight: 500,
  },
};

const ListSubheader = React.createClass({
  propTypes: {
    subheader: React.PropTypes.string.isRequired,
  },
  mixins: [
    PureRenderMixin,
  ],
  render() {
    return (
      <div style={styles.root} data-test="ListSubheader">
        {this.props.subheader}
      </div>
    );
  },
});

export default ListSubheader;

@newoga
Copy link
Contributor

newoga commented Jan 22, 2016

@oliviertassinari I agree that this could get it's own section in the documentation like <Divider />. This component can be used with List, Menu, and Grid.

@mbrookes
Copy link
Member

It might make sense to add Subheader to one of the examples for each component this can be used from and cross reference it from there to the Subheader docs. I can pick this up once the PR is accepted.

@newoga
Copy link
Contributor

newoga commented Jan 22, 2016

Thanks @mbrookes, that'd be awesome 👍

Once we're squashed, I'm good with this being merged and @mbrookes can tackle the docs for it separately.

@pradel
Copy link
Contributor Author

pradel commented Jan 22, 2016

Hum sorry I think I made a mistake when I try to rebase can you guide me for that ?

@newoga
Copy link
Contributor

newoga commented Jan 23, 2016

@pradel Are you familiar with interactive rebase?

## Add callemall/material-ui as remote
git remote add upstream git://github.com/callemall/material-ui.git

## fetch
git fetch upstream

## Rebase onto upstream master
git pull --rebase upstream master

## Rebase interactively (for squash)
git rebase -i upstream master

# Now you're in interactive rebase...
# (1) squash first two commits, save
# (2) edit commit message, save

# push remotely
git push -f origin

mbrookes and others added 10 commits January 23, 2016 11:27
Added GridList subheader

- Added
https://www.google.com/design/spec/components/subheaders.html#subheaders
-grid-subheaders
- Updated docs example

make a separate Subheader component

remove subheader prop from GridList

make a separate Subheader component

Added GridList subheader

- Added
https://www.google.com/design/spec/components/subheaders.html#subheaders
-grid-subheaders
- Updated docs example

make a separate Subheader component

make a separate Subheader component

make a separate Subheader component
…Item] When checkbox className is not returned
This component really should have had a deprecation warning from 0.14.0 when the old `./menu/menu` implementation was deprecated. This component is not used in any other components in src or docs. It also is not documented on the site. I think we should add this warning ASAP and plan for removal along with `./menu/menu` in our first `0.15.0` release candidate.
@oliviertassinari oliviertassinari 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 Jan 23, 2016
@newoga
Copy link
Contributor

newoga commented Jan 24, 2016

@pradel Looks like something went wrong when you tried squashing. Do you still have your changes somewhere?

@alitaheri
Copy link
Member

@pradel Just to ease your mind. nothing ever gets deleted from git unless you brutally force it 😆

But now you have lost a pointer to your commits. you can take a look at your logs to see where you were before the rebase and make a branch on that particular commit. if you have a hard time finding it. you can email me your .git folder so i can fix this for you.

@mbrookes
Copy link
Member

git reflog is your friend here.

@pradel
Copy link
Contributor Author

pradel commented Jan 24, 2016

@newoga yes I still have the changes :)
@alitaheri I send you a email with my .git folder because I try a lot of things but I still don't succeed to fix this thanks !

@newoga
Copy link
Contributor

newoga commented Jan 24, 2016

@pradel Thanks for your work on this!

@alitaheri
Copy link
Member

@pradel I got your email. I'll have a look thank 😁

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
PR: needs revision The pull request can't be merged. More details is available on the code review or fails in the CI
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

10 participants