[RaisedButton] Respect theme fontSize#3988
Conversation
I'm using the |
| verticalAlign: 'middle', | ||
| opacity: 1, | ||
| fontSize: '14px', | ||
| fontSize: raisedButton.fontSize || button.fontSize || '14px', |
There was a problem hiding this comment.
I think that we should follow the FlatButton implementation.
What about applying the fontSize at the root of the component and adding raisedButton.fontSize: typography.fontStyleButtonFontSize into getMuiTheme.js?
Then using this using this value here instead of the hard coded 14px?
|
@echenley are you still working on this? |
|
@mbrookes Ah sorry, I forgot about this. I'll take a look today. |
|
@mbrookes This should work. Do you think it's worth adding a test for it? |
echenley
force-pushed
the
bugfix/raised-button-font-size
branch
from
8381bb3 to
2292790
Compare
|
Thanks @echenley tests are always welcome, so if you wouldn't mind, that would be great! |
|
Also, since the commit history is most just rebases etc, you can go ahead and squash these, and the test when it's ready. |
echenley
force-pushed
the
bugfix/raised-button-font-size
branch
from
2292790 to
5590584
Compare
| position: 'relative', | ||
| opacity: 1, | ||
| fontSize: '14px', | ||
| fontSize: raisedButton.fontSize || button.fontSize, |
There was a problem hiding this comment.
@mbrookes Now that I look at it, there's no point in keeping || button.fontSize, since raisedButton.fontSize will always be defined, right? Unless someone purposefully sets it to something falsy.
echenley
force-pushed
the
bugfix/raised-button-font-size
branch
from
5590584 to
ea30623
Compare
|
Should be good to go! |
| "eslint-plugin-react": "^5.0.1", | ||
| "highlight.js": "^9.0.0", | ||
| "history": "^2.0.0", | ||
| "html-webpack-plugin": "^2.16.1", |
There was a problem hiding this comment.
Ah, actually, that's my mistake. I need to update the webpack dev-server config to match the production one.
There was a problem hiding this comment.
Gotcha, I removed the commit!
echenley
force-pushed
the
bugfix/raised-button-font-size
branch
from
ea30623 to
844f56a
Compare
|
Looks good. Thanks @echenley 👍 |
| }); | ||
|
|
||
| it('inherits fontSize from theme', () => { | ||
| const wrapper = mountWithContext( |
There was a problem hiding this comment.
Could we use the shallow rendering instead? I don't think that we need to mount the component to test this behavior.
There was a problem hiding this comment.
Unfortunately not. Shallow rendering doesn't go deep enough to render the label. Edit: actually, I may be wrong.
There was a problem hiding this comment.
Reading at the source code. I think that we can.
Even if we were using an internal RaisedButtonLabel, we could just make sure the right style props is applied.
echenley
force-pushed
the
bugfix/raised-button-font-size
branch
2 times, most recently
from
ea52377 to
884c36d
Compare
| <RaisedButton label="test" /> | ||
| ); | ||
|
|
||
| assert.strictEqual(wrapper.contains('test'), true); |
There was a problem hiding this comment.
What about this? wrapper.text() doesn't work since there are non-primitive components in the way (it just returns '<Paper />'). The alternative is something like wrapper.find('EnhancedButton').childAt(0).text().
|
@echenley The commit message isn't following the PR title. Aside from this minor issue. That looks great 👍. |
echenley
force-pushed
the
bugfix/raised-button-font-size
branch
from
884c36d to
54d8223
Compare
echenley
changed the title
|
@oliviertassinari Oops, updated and removed the redundancy! |
|
@echenley Thanks! 👍 |
zannager
added
scope: button
4 participants
RaisedButton label fontSize is hard coded as 14px. This PR allows the theme to override it.
Quick question: Is this the way you guys are doing it? There are styles all over that don't inherit from the theme, and this seems like a pretty heavy-handed way of dealing with it. I do see why it's complicated with multiple nested elements inheriting from a single config object. Just wondering what the idea is for this. Thanks!