Update PageHeader to render subtitle as an h2#106
Conversation
|
LGTM |
|
@hturan could lay out your reasoning behind the change. |
|
It looks like CI is failing on a completely separate suite; @manatarms Ah ace, sorry for not checking! Feel free to make the change and close this PR when that lands 👍 This isn't super urgent, but let me know roughly how long you think your PR will take to land. @sejoker There are a variety of reasons, but mainly: if |
| <h1 className="cf-page__title">{this.props.title}</h1> | ||
| {this.props.subtitle && | ||
| <p className="cf-page__subtitle">{this.props.subtitle}</p>} | ||
| <h2 className="cf-page__subtitle">{this.props.subtitle}</h2>} |
There was a problem hiding this comment.
Why not cf-component-heading?
There was a problem hiding this comment.
@jwineman I like the idea of using cf-component-heading, but it would introduce a dependency in cf-component-page. So it depends on if we want that.
There was a problem hiding this comment.
I think we should avoid it whenever possible. However we have instances where our components depend on each other. cf-component-card has a dependency on cf-component link for example.
If we expect this <h2> to look and function the same as cf-component-heading we should introduce that dependency here since not doing so makes this use a customization that will limit its ability to get future updates we make to that component. This has technically already happened since cf-component-heading has been converted to Fela but this component is still using hard coded CSS classes.
There was a problem hiding this comment.
This is actually the right approach. There's only one kind of PageHeader, and the default styles in cf-component-heading are not contextual. <Heading size={2}> can mean very different things in different circumstances. There's no one size fits all approach here, as evident by the <h1> above.
Yea, it's a flaky test that sometimes time outs. :-/ |
|
@hturan I reran travis. Works this time. |
|
Don't merge this because I'm working on making PageHeader use Heading. |
| <h1 className="cf-page__title">{this.props.title}</h1> | ||
| {this.props.subtitle && | ||
| <p className="cf-page__subtitle">{this.props.subtitle}</p>} | ||
| <h2 className="cf-page__subtitle">{this.props.subtitle}</h2>} |
There was a problem hiding this comment.
This is actually the right approach. There's only one kind of PageHeader, and the default styles in cf-component-heading are not contextual. <Heading size={2}> can mean very different things in different circumstances. There's no one size fits all approach here, as evident by the <h1> above.
|
Any updates on this? @manatarms it seems that #102 has been closed so I assume it's not blocking any more? |
|
I think this is ok to merge. |
|
@hturan Can we merge this or has this passed its last-sell-by date? 💩 |
|
@koddsson I don't even know any more. Let's ship it! |
|
|
|
Fuuuuuu, this is the wrong branch innit? |
7 participants
This is a super-simple SEMVER-major PR to make
cf-component-page'sPageHeaderrender it'ssubtitleprop as anh2.I've run
npm run update-snapshotand committed the changes forcf-component-page. Is this correct? I couldn't find any documentation about how to do this so just picked the most relevantscript😄