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

[List] Remove Paper #3612

Merged
merged 1 commit into from
Mar 10, 2016
Merged

[List] Remove Paper #3612

merged 1 commit into from
Mar 10, 2016

Conversation

mbrookes
Copy link
Member

@mbrookes mbrookes commented Mar 7, 2016

  • PR has tests / docs demo, and is linted.
  • Commit and PR titles begin with [ComponentName], and are in imperative form: "[Component] Fix leaky abstraction".
  • Description explains the issue / use-case resolved, and auto-closes the related issue(s) (http://tr.im/vFqem).

Paper is redundant, as, like Menu, List is used wrapped in other components such as LeftNav, which may already use Paper.

@alitaheri
Copy link
Member

This is good 👍 But it's a breaking change we should document it and update the examples too. also, zDepth should error as it's functionality is not deprecated but rather removed

@mbrookes
Copy link
Member Author

mbrookes commented Mar 7, 2016

All good points - I think I rushed this! 😄

@mbrookes 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 Refactoring labels Mar 7, 2016
@newoga
Copy link
Contributor

newoga commented Mar 8, 2016

@mbrookes @alitaheri I know conceptually this sounds weird/crazy, but if we made this change, we could probably just deprecate List altogether, since List would essentially be an alias/wrapping of a standard <div />.

Users could nest ListItem components in a standard <div /> or a <Paper /> element themselves without List.

Is that too weird?

I guess another option to make it less weird is we could get rid of the selectable-enhance HOC and implement the selectable functionality in the List component. So if you selectable behavior, use List, if not, just use <Paper /> and <ListItem />. Are there any other use cases for selectable-enhance other than List?

@mbrookes
Copy link
Member Author

mbrookes commented Mar 8, 2016

we could probably just deprecate List altogether

I thought the same thing when I first looked at this - about the only thing that is left is some padding, which varies depending on whether there's a Subheader, however it does seem a bit unnatural not to have a List component to wrap ListItem.

Are there any other use cases for selectable-enhance other than List?

It looks fairly tightly coupled to List, although I'm curious, as it's been named and located as if it's a standalone component.

There was a request on gitter today for a multi-select menu, but I'm not sure how useful this would be there.

I do intend to revisit this PR, but last time I tried to update it, I was plagued by the inline-style-prefixer issue!

@newoga
Copy link
Contributor

newoga commented Mar 8, 2016

I do intend to revisit this PR, but last time I tried to update it, I was plagued by the inline-style-prefixer issue!

I hear you! 😄

It looks fairly tightly coupled to List, although I'm curious as it's been named and located as if it's a standalone component.

I've thought about it some more, I think my vote is let's expand List to include some the selection functionality. I think it makes sense. Menu and List are related to each other, but Menu currently manages it's own selection state and I don't think it will ever make sense use selectable-enhance with it.

Also, having a parent List component may help one day if we want to support something like Leave Behinds.

I think if we get the API right, the Menu components could be dramatically simplified because right now there is a lot of duplicating functionality/API.

@mbrookes
Copy link
Member Author

mbrookes commented Mar 9, 2016

zDepth should error as it's functionality is not deprecated but rather removed

@alitaheri That's a fair point. I wanted to show some kind of meaningful message, so rather than just remove the prop I've used warning directly.

Let me know if you'd suggest something different. I think a warning is reasonable, as nothing will break if the prop is provided, it just won't generate a shadow if one was expected.

Most devs will be using List inside LeftNav or Popover anyway.

@newoga I think your suggestions deserve their own PR, perhaps combined with deprecating valueLink from the HOC.

@mbrookes 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 Mar 9, 2016
@alitaheri
Copy link
Member

I think a warning is reasonable, as nothing will break if the prop is provided, it just won't generate a shadow if one was expected.

Good point 👍 And the warning explicitly expresses that it's removed not deprecated 👍 👍

@alitaheri
Copy link
Member

This looks good 😅

@nathanmarks
Copy link
Member

Whoo!

@nathanmarks
Copy link
Member

@mbrookes Mind squashing it?

@nathanmarks nathanmarks added the PR: needs revision The pull request can't be merged. More details is available on the code review or fails in the CI label Mar 10, 2016
@oliviertassinari
Copy link
Member

I really like the direction this PR is taking 👍. Let's split the concern of each component.

@oliviertassinari oliviertassinari removed the PR: needs revision The pull request can't be merged. More details is available on the code review or fails in the CI label Mar 10, 2016
@alitaheri
Copy link
Member

Thanks a lot @mbrookes 👍 👍

mbrookes added a commit that referenced this pull request Mar 10, 2016
@mbrookes mbrookes merged commit d768a13 into mui:master Mar 10, 2016
@alitaheri
Copy link
Member

Ops, I forgot to push the merge button.

@zannager zannager added the component: list This is the name of the generic UI component, not the React module! label Mar 14, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
component: list This is the name of the generic UI component, not the React module!
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants