Fix for react troubleshooting component heading style#5630
Conversation
…ter a quick discussion added the missing style class and the ability to customize the header tag in the component.
zachmargolis
left a comment
zachmargolis
left a comment
There was a problem hiding this comment.
LGTM but it looks like the headingTag option is never used? Are we planning to use it in the future?
| function TroubleshootingOptions({ headingTag = 'h2', headingText, options }) { | ||
| const HeadingTag = headingTag; | ||
|
|
||
| return ( |
There was a problem hiding this comment.
I think we can rename this so we don't need the separate assignment
| function TroubleshootingOptions({ headingTag = 'h2', headingText, options }) { | |
| const HeadingTag = headingTag; | |
| return ( | |
| function TroubleshootingOptions({ headingTag: HeadingTag = 'h2', headingText, options }) { | |
| return ( |
There was a problem hiding this comment.
It is not currently used but will be in the future. I can cut it out but it'll be needed when the rest of the IAL2 flow is moved into react.
| * @param {TroubleshootingOptionsProps} props | ||
| */ | ||
| function TroubleshootingOptions({ heading, options }) { | ||
| function TroubleshootingOptions({ headingTag = 'h2', headingText, options }) { |
There was a problem hiding this comment.
Could we call it headingLevel to match the ERB partial? Or vice-versa, update the ERB partial to match this name?
I suppose an argument could be made that "heading level" would be the numeric value, not the tag name, so either renaming both as "heading tag", or using "heading level" as a numeric value.
| * @typedef TroubleshootingOptionsProps | ||
| * | ||
| * @prop {string} heading | ||
| * @prop {string=} headingTag |
There was a problem hiding this comment.
For your lint error, I think TypeScript is upset about accepting an arbitrary string as a tag name. It works better for me if you explicitly make it one of the heading tags we're expecting:
| * @prop {string=} headingTag | |
| * @prop {'h1'|'h2'|'h3'|'h4'|'h5'|'h6'=} headingTag |
There was a problem hiding this comment.
Makes sense. I was thinking about the arbitrary nature of it but wasn't sure exactly how to address it in TS.
…tions of the troubleshooting component.
| /** | ||
| * @typedef TroubleshootingOptionsProps | ||
| * | ||
| * @prop {string} heading |
There was a problem hiding this comment.
Should we bring this option (back into) alignment b/w JSX and ERB?
There was a problem hiding this comment.
fixed in 6516040
|
|
||
| const heading = getByRole('heading'); | ||
|
|
||
| expect(getByText('Need help?').tagName).to.be.equal('H2'); |
There was a problem hiding this comment.
Could this have used existing heading variable? Should be the same?
| expect(getByText('Need help?').tagName).to.be.equal('H2'); | |
| expect(heading.tagName).to.be.equal('H2'); |
There was a problem hiding this comment.
Oops, I moved that test to a different test at the last minute and didn't really notice the heading was already pulled there.
3 participants
There was a styling bug in the TroubleShooting React component. So after a quick discussion the missing style class was added and the ability to customize the header tag in the component was introduced. This brings the react troubleshooting component in line with the ruby implementation.