adds prop to override h2 element #3585
Conversation
rhenshaw56
added
ws-articles
| dir="ltr" | ||
| id="test-section-label" | ||
| > | ||
| This is text in a SectionLabel. | ||
| </span> | ||
| <span | ||
| aria-hidden="true" | ||
| class="c6" | ||
| class="c7" | ||
| dir="ltr" | ||
| > | ||
| See All |
There was a problem hiding this comment.
If the journalist inputs a link - we won't want this in the recommendations component... will it be possible for a journalist to input a link?
There was a problem hiding this comment.
@greenc05 in the recommendations container in simorgh, it will be hard coded to not have a link text or an href, so we never have this outputted as a link
There was a problem hiding this comment.
Thanks @rhenshaw56 maybe we don't need tests for this scenario then?
| @@ -133,6 +135,7 @@ SectionLabel.propTypes = { | |||
| service: string.isRequired, | |||
| visuallyHidden: bool, | |||
| backgroundColor: string, | |||
| overrideHeadingAs: string, | |||
There was a problem hiding this comment.
should we restrict the allowed value for this to be only null or strong at this time, just to discourage misuse and encourage people to choose the correct element type in the future?
There was a problem hiding this comment.
If that is possible, definately
There was a problem hiding this comment.
@andrewscfc would a prop validation warning suffice? or should it be strictly enforced?
There was a problem hiding this comment.
I'd just say prop validation personally, just to guide people in the right direction. Enforcing is a bit pointless because anyone can edit the code, it's just to encourage people to pause and think really.
| @@ -81,11 +81,12 @@ const SectionLabel = ({ | |||
| service, | |||
| visuallyHidden, | |||
| backgroundColor, | |||
| overrideHeadingAs, | |||
There was a problem hiding this comment.
When this is used in Simorgh could we summarise why we are overriding to strong in a comment I don't think this will be very obvious when people come to this in the future #3574 (comment)
I was considering suggesting adding a supplementary prop, this trying to express the reason for using strong, the section label being inside supplementary content rather than being used in the main content of the page; this could then be used inside the component to switch to use a strong. I'm still not sure this is clear enough though, so I'd personally just suggest adding a comment as above.
| </div> | ||
| `; | ||
|
|
||
| exports[`SectionLabel With heading overriden to be a strong element should render correctly as a plain title with no bar 1`] = ` |
There was a problem hiding this comment.
whats the difference in this to the example above?
There was a problem hiding this comment.
@greenc05 none actually, just a change in the test description title.
|
Looks good to me. Looked at Storybook and we have an option under sectionlabel for 'with heading overriden to be a strong element' |
Resolves #3574
Overall change: Optionally render any element instead of an h2.
Code changes: