Updates Page and PageHeader components to use Fela#102
Updates Page and PageHeader components to use Fela#102
Conversation
|
Just a reminder. Don't merge this till we know what to do with Fela components. |
|
please rebase |
| ); | ||
| } | ||
| } | ||
| const styles = props => { |
There was a problem hiding this comment.
const styles = () => ({})
is more concise.
| ); | ||
| } | ||
| } | ||
| const styles = props => { |
There was a problem hiding this comment.
I recommend using destructuring:
const styles = ({ theme }) => ({
'& h1': {
marginBottom: theme.marginBottom
},
'& p': {
marginTop: theme.marginTop,
fontSize: theme.fontSize,
color: theme.defaultColor
}
})
| const PageHeader = ({ title, subtitle, className }) => ( | ||
| <header className={className}> | ||
| <h1 className={className}>{title}</h1> | ||
| {subtitle && <p className={className}>{subtitle}</p>} |
There was a problem hiding this comment.
before your changes header and subtitle had different classNames, now you apply the same. It won't work like you expect.
| exports[`should render 1`] = ` | ||
| <article | ||
| className="cf-page" | ||
| className="" |
There was a problem hiding this comment.
this is an indication that something is broken, fela should always assign a className value.
|
In case you need to assign multiple classNames to one React Component, you should split it into multiple React components. One component = 1 classname. |
Not true anymore. I've added Anyway, your current solution doesn't work. You can't assign the same className to different nodes. |
|
Should I still make these changes in this PR? Or open a new one with the updated migration changes? |
wyuenho
left a comment
wyuenho
left a comment
There was a problem hiding this comment.
Convert this back to ES6 classes please. We also export the styled component by default now. The theme is also sorely missing a lot of styles.
|
Also, please coordinate with #106 |
|
Closing this out because @jwineman is working with design to break up heading into distinct components. |
4 participants
Few things to Note in this PR.
The Page component does not have any styles that we apply to it specifically. I have put in the scaffolding for Fela, in case we decide to put in styles during backbone-react migration.
If someone has objections to this I will revert to the original component, but it wont support Fela.