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.

Sign up for GitHub

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

Jump to bottom

[Menu][DropDownMenu][SelectField] Add menuItemStyle and menuItemSelectedStyle properties #5389

Merged
merged 10 commits into from
Dec 16, 2016
Merged

[Menu][DropDownMenu][SelectField] Add menuItemStyle and menuItemSelectedStyle properties #5389

merged 10 commits into from
Dec 16, 2016

Conversation

DoubleU23
Copy link

@DoubleU23 DoubleU23 commented Oct 13, 2016 •
edited
Loading

related PR's
continuation/extension of PR #5379
supersedes PR #5091

related issues
#5166, #3388

DropDownMenu/SelectField and the underlying Menu
now have additional props: menuItemStyle and menuItemSelectedStyle

menuItemSelectedStyle
supersedes selectedMenuItemStyle (it was never applied)
prop to overwrite the style of the selected menuItem

menuItemStyle
not neccessary and may be removed.
i added this aswell, just for convention.
my thoughts on this:
f.e.: if the menuItems are rendered by a helper function from a seperate file...
if a user wants to style the selecteMenuItem he probably wants to style the other items aswell on the same file/location and not in the helper.

  • 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).

@muibot muibot added PR: Needs Review 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 Oct 13, 2016
This was referenced Oct 13, 2016
Closed
Merged
@elodszopos
Copy link

👍 I was just trying to use what you had in the previous PR and wondering why it wasn't working. Started debugging and realized I don't have the code yet. Duh'.

Thank you for this. Finally I can easily put all the pink to rest.

@oliviertassinari
Copy link
Member

oliviertassinari commented Oct 16, 2016 •
edited
Loading

I feel like we should change the API for the SelectField and the DropDownMenu.
What if users were able to provide arbitrary properties to the DropDownMenu and respectively the Menu component?
I'm quite worried by the overhead of adding a new property everytime one user want to access a deep down component. I don't think that we should make everybody pay for it.

We could have something close to the menuProps.

@oliviertassinari oliviertassinari added component: DropDownMenu component: menu This is the name of the generic UI component, not the React module! component: select This is the name of the generic UI component, not the React module! labels Nov 6, 2016
@oliviertassinari oliviertassinari mentioned this pull request Nov 6, 2016
Closed
@chrisajimenez
Copy link

What needs to be revised here? I'd love to help. I agree that creating hooks for every deep down component will probably not be a great long term solution, but it is annoying that there is no current way to change the colors of those selected menu items currently, without css. This PR seems to be a good solution until API changes can be agreed upon.

oliviertassinari
oliviertassinari requested changes Dec 10, 2016
View reviewed changes
Copy link
Member

@oliviertassinari oliviertassinari left a comment •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm quite worried by the overhead of adding a new property everytime one user want to access a deep down component. I don't think that we should make everybody pay for it.
We could have something close to the menuProps.

I have changed my mind as the style property requires some specific logic.
My only concern regarding merging this PR is with the breaking change. I would rather keep the selectedMenuItemStyle property.

@chrisajimenez Do you have enough information to continue that PR? Thanks for proposing your help.

src/DropDownMenu/DropDownMenu.js
@@ -315,6 +325,8 @@ class DropDownMenu extends Component {
style={menuStyle}
listStyle={listStyle}
onItemTouchTap={this.handleItemTouchTap}
menuItemStyle={menuItemStyle}
menuItemSelectedStyle={menuItemSelectedStyle}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What about?

selectedMenuItemStyle={selectedMenuItemStyle}

src/Menu/Menu.js
@@ -112,10 +120,6 @@ class Menu extends Component {
/** @ignore */
onKeyDown: PropTypes.func,
/**
* Override the inline-styles of selected menu items.
*/
selectedMenuItemStyle: PropTypes.object,
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would rather keep that name to not introduce breaking changes.

oliviertassinari
oliviertassinari approved these changes Dec 14, 2016
View reviewed changes
Copy link
Member

@oliviertassinari oliviertassinari left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That looks good aside from my two small comments.

src/Menu/Menu.js
@@ -111,7 +115,7 @@ class Menu extends Component {
onItemTouchTap: PropTypes.func,
/** @ignore */
onKeyDown: PropTypes.func,
/**
/**
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

space typo

src/Snackbar/Snackbar.js
@@ -17,17 +17,27 @@ function getStyles(props, context, state) {

const {open} = state;

const {position} = props;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks like change on the Snackbar wasn't intended to be in that PR.

Copy link
Author

@DoubleU23 DoubleU23 Dec 16, 2016 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

oh no, sry.
that were the changes of #5437
because i already merged it into master in my branch...
=> set it to actual content of Snackbar.js

@oliviertassinari oliviertassinari removed PR: Needs Review PR: needs revision The pull request can't be merged. More details is available on the code review or fails in the CI labels Dec 16, 2016
@oliviertassinari oliviertassinari merged commit d97ebad into mui:master Dec 16, 2016
@oliviertassinari
Copy link
Member

@DoubleU23 Thanks!

@DoubleU23 DoubleU23 deleted the Menu/menuItemSelectedStyle branch December 23, 2016 13:31
@oliviertassinari oliviertassinari changed the title [Menu][DropDownMenu][SelectField] menuItemStyle & menuItemSelectedStyle [Menu][DropDownMenu][SelectField] Add menuItemStyle and menuItemSelectedStyle properties Dec 25, 2016
@oliviertassinari oliviertassinari mentioned this pull request Jan 1, 2017
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@oliviertassinari oliviertassinari oliviertassinari approved these changes

Assignees
No one assigned
Labels
component: menu This is the name of the generic UI component, not the React module! component: select This is the name of the generic UI component, not the React module!
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@DoubleU23 @elodszopos @oliviertassinari @chrisajimenez @muibot