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

[AppBar] Add iconStyleLeft prop #3668

Closed
wants to merge 1 commit into from
Closed

[AppBar] Add iconStyleLeft prop #3668

wants to merge 1 commit into from

Conversation

59naga
Copy link
Contributor

@59naga 59naga commented Mar 11, 2016

#3667

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

Fix #3667

@mbrookes mbrookes changed the title [AppBar] Add iconStyleLeft prop (fix #3667) [AppBar] Add iconStyleLeft prop Mar 11, 2016
@alitaheri alitaheri added PR: Review Accepted 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: Review Accepted labels Mar 11, 2016
@@ -260,7 +268,7 @@ const AppBar = React.createClass({
}

menuElementLeft = (
<div style={prepareStyles(Object.assign({}, styles.iconButtonStyle))}>
<div style={prepareStyles(iconLeftStyle)}>
Copy link
Member

Choose a reason for hiding this comment

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

Might this be a regression? Why even change these?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

iconRightStyle will change the style of menuElementRight.
iconLeftStyle should also be similar.

Copy link
Member

Choose a reason for hiding this comment

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

Yeah, but this is a breaking change. you shouldn't remove the styles.iconButtonStyle just override it: Object.assign({}, styles.iconButtonStyle, iconLeftStyle)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Oh, I'm sorry. Including a test, and re-commit again at a later date. 😵

@@ -247,6 +253,8 @@ const AppBar = React.createClass({
style: prepareStyles(Object.assign(styles.title, styles.mainElement, titleStyle)),
}, title);

const iconLeftStyle = Object.assign({}, styles.iconButtonStyle, iconStyleLeft);
Copy link
Member

Choose a reason for hiding this comment

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

you shouldn't do this either as it's resulting in calling prepareStyles twice. Please follow my comments as they are 😁

@nathanmarks
Copy link
Member

@alitaheri @newoga This component is way too rigid.

@newoga
Copy link
Contributor

newoga commented Mar 13, 2016

@alitaheri @newoga This component is way too rigid.

I agree, are we still going to try to merge the new AppBar this release?

@59naga
Copy link
Contributor Author

59naga commented Mar 17, 2016

👍 yeah, would you like any improvements to other?

import IconButton from 'src/icon-button';
import Paper from 'src/paper';
import NavigationClose from 'src/svg-icons/navigation/close';
import 'src/';
Copy link
Member

Choose a reason for hiding this comment

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

What is this for?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

this is a typo, it was deleted 😅

- Add export `getStyles` of app-bar.js for test
@mbrookes mbrookes assigned alitaheri and newoga and unassigned alitaheri Mar 28, 2016
@nathanmarks nathanmarks added the PR: out-of-date The pull request has merge conflicts and can't be merged label May 15, 2016
@nathanmarks
Copy link
Member

@newoga what's the situ with this PR?

@59naga 59naga mentioned this pull request May 15, 2016
4 tasks
@59naga
Copy link
Contributor Author

59naga commented May 15, 2016

Updated PR. would appreciate your review.

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 PR: out-of-date The pull request has merge conflicts and can't be merged
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants