Skip to content

Fix for react troubleshooting component heading style #5630

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
amathews-fs merged 6 commits into from
Nov 23, 2021
Merged

Fix for react troubleshooting component heading style #5630

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
0a6737a
There was a styling bug in the TroubleShooting react component. So af…
amathews-fs Nov 22, 2021
770f32b
Possible lint fix?
amathews-fs Nov 22, 2021
6077819
Lint fix and bringing naming conventions in line with both implementa…
amathews-fs Nov 23, 2021
9e15ca0
Update JS tests for troubleshoot-options.
amathews-fs Nov 23, 2021
6516040
Changing JS variable name to keep it aligned with ruby.
amathews-fs Nov 23, 2021
188b642
Minor test cleanup.
amathews-fs Nov 23, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
9 changes: 6 additions & 3 deletions app/javascript/packages/components/troubleshooting-options.jsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -11,17 +11,20 @@ import { BlockLink } from '@18f/identity-components';
/**
* @typedef TroubleshootingOptionsProps
*
* @prop {string} heading

@aduth aduth Nov 23, 2021

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should we bring this option (back into) alignment b/w JSX and ERB?

@amathews-fs amathews-fs Nov 23, 2021

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

fixed in 6516040

* @prop {string=} headingTag

@aduth aduth Nov 23, 2021

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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:

Suggested change
* @prop {string=} headingTag
* @prop {'h1'|'h2'|'h3'|'h4'|'h5'|'h6'=} headingTag

@amathews-fs amathews-fs Nov 23, 2021

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Makes sense. I was thinking about the arbitrary nature of it but wasn't sure exactly how to address it in TS.

* @prop {string} headingText
* @prop {TroubleshootingOption[]} options
*/

/**
* @param {TroubleshootingOptionsProps} props
*/
function TroubleshootingOptions({ heading, options }) {
function TroubleshootingOptions({ headingTag = 'h2', headingText, options }) {

@aduth aduth Nov 23, 2021

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

const HeadingTag = headingTag;

return (

@zachmargolis zachmargolis Nov 22, 2021

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we can rename this so we don't need the separate assignment

Suggested change
function TroubleshootingOptions({ headingTag = 'h2', headingText, options }) {
const HeadingTag = headingTag;
return (
function TroubleshootingOptions({ headingTag: HeadingTag = 'h2', headingText, options }) {
return (

@amathews-fs amathews-fs Nov 22, 2021

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

<section className="troubleshooting-options">
<h2>{heading}</h2>
<HeadingTag className="troubleshooting-options__heading">{headingText}</HeadingTag>
<ul className="troubleshooting-options__options">
{options.map(({ url, text, isExternal }) => (
<li key={url}>
Expand Down
4 changes: 2 additions & 2 deletions app/javascript/packages/components/troubleshooting-options.spec.jsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,7 @@ import TroubleshootingOptions from './troubleshooting-options';

describe('TroubleshootingOptions', () => {
it('renders a given heading', () => {
const { getByRole } = render(<TroubleshootingOptions heading="Need help?" options={[]} />);
const { getByRole } = render(<TroubleshootingOptions headingText="Need help?" options={[]} />);

const heading = getByRole('heading');

Expand All @@ -13,7 +13,7 @@ describe('TroubleshootingOptions', () => {
it('renders given options', () => {
const { getAllByRole } = render(
<TroubleshootingOptions
heading=""
headingText=""
options={[
{ text: <>Option 1</>, url: 'https://example.com/1', isExternal: true },
{ text: 'Option 2', url: 'https://example.com/2' },
Expand Down
2 changes: 1 addition & 1 deletion app/javascript/packages/document-capture/components/warning.jsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -54,7 +54,7 @@ function Warning({
)}
{troubleshootingOptions && (
<TroubleshootingOptions
heading={troubleshootingHeading || t('idv.troubleshooting.headings.having_trouble')}
headingText={troubleshootingHeading || t('idv.troubleshooting.headings.having_trouble')}
options={troubleshootingOptions}
/>
)}
Expand Down