Skip to content
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.

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

Add custom heading level option to radio component #1951

Merged
merged 2 commits into from
Mar 2, 2021

Conversation

danacotoran
Copy link
Contributor

If the radio component is passed a heading, and the heading is not a "Page heading", then that heading is always a h2.

This is not always appropriate; there are instances where more flexibility is necessary in order to maintain a correct hierarchical heading structure.

For example, on the Account information consent page:
Screenshot 2021-03-01 at 16 20 45

This introduces the shared helper's heading level function to the radio component. This way we can ensure the default
remains h2, while also having the ability to pass a custom heading level where necessary.

Relevant card: https://trello.com/c/lNkwruEi/647-fix-accessibility-issues-raised-by-dac

@bevanloon bevanloon temporarily deployed to govuk-publis-custom-hea-vizvt1 March 1, 2021 16:26 Inactive
@bevanloon bevanloon temporarily deployed to govuk-publis-custom-hea-vizvt1 March 1, 2021 16:33 Inactive
Copy link
Contributor

@andysellick andysellick left a comment

Choose a reason for hiding this comment

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

Looks good, +1 for using the shared helper. I've approved but noted a few minor things.

Currently if the radio component is passed a heading, and the heading is
not a "Page heading", then that heading is always a h2.
This is not always appropriate; there are instances where more
flexibility is necessary in order to maintain a correct hierarchical
heading structure.
This introduces the shared helper's heading level function to the radio
component. This way we can ensure the default remains h2, while also
having the ability to pass a custom heading level where necessary.
@danacotoran danacotoran force-pushed the custom-heading-lvl-for-radio branch from 1d95d90 to 8e90d32 Compare March 2, 2021 14:50
@bevanloon bevanloon temporarily deployed to govuk-publis-custom-hea-vizvt1 March 2, 2021 14:50 Inactive
@danacotoran danacotoran merged commit 9232daf into master Mar 2, 2021
@danacotoran danacotoran deleted the custom-heading-lvl-for-radio branch March 2, 2021 14:58
@danacotoran danacotoran mentioned this pull request Mar 2, 2021
This was referenced Mar 11, 2021
danacotoran added a commit that referenced this pull request May 6, 2021
This is step 1 towards removing the is_page_heading parameter from the
radio component. The reason behind this is that the ability to pass a
custom heading level was introduced to the radio component some time
back #1951
The is_page_heading parameter is a specialised parameter that dictates
that the heading should be a h1 of size xl.
This used to solve a problem when the component did not use to support
heading_level, however now that we can use heading_level and
heading_size to achieve the same result, the is_page_heading parameter
becomes redundant.

The reason we have to do this in steps is that the component is
referenced with the is_page_heading parameter in several applications.
Due to the way the logic in this component is structured, it would be
a breaking change if we simply went ahead with replacing
is_page_heading: true with heading_level: 1 and heading_size: 'xl'
danacotoran added a commit that referenced this pull request May 7, 2021
This is step 1 towards removing the is_page_heading parameter from the
radio component. The reason behind this is that the ability to pass a
custom heading level was introduced to the radio component some time
back #1951
The is_page_heading parameter is a specialised parameter that dictates
that the heading should be a h1 of size xl.
This used to solve a problem when the component did not use to support
heading_level, however now that we can use heading_level and
heading_size to achieve the same result, the is_page_heading parameter
becomes redundant.

The reason we have to do this in steps is that the component is
referenced with the is_page_heading parameter in several applications.
Due to the way the logic in this component is structured, it would be
a breaking change if we simply went ahead with replacing
is_page_heading: true with heading_level: 1 and heading_size: 'xl'
danacotoran added a commit that referenced this pull request May 7, 2021
This is step 1 towards removing the is_page_heading parameter from the
radio component. The reason behind this is that the ability to pass a
custom heading level was introduced to the radio component some time
back #1951
The is_page_heading parameter is a specialised parameter that dictates
that the heading should be a h1 of size xl.
This used to solve a problem when the component did not use to support
heading_level, however now that we can use heading_level and
heading_size to achieve the same result, the is_page_heading parameter
becomes redundant.

The reason we have to do this in steps is that the component is
referenced with the is_page_heading parameter in several applications.
Due to the way the logic in this component is structured, it would be
a breaking change if we simply went ahead with replacing
is_page_heading: true with heading_level: 1 and heading_size: 'xl'
danacotoran added a commit that referenced this pull request May 7, 2021
This is step 1 towards removing the is_page_heading parameter from the
radio component. The reason behind this is that the ability to pass a
custom heading level was introduced to the radio component some time
back #1951
The is_page_heading parameter is a specialised parameter that dictates
that the heading should be a h1 of size xl.
This used to solve a problem when the component did not use to support
heading_level, however now that we can use heading_level and
heading_size to achieve the same result, the is_page_heading parameter
becomes redundant.

The reason we have to do this in steps is that the component is
referenced with the is_page_heading parameter in several applications.
Due to the way the logic in this component is structured, it would be
a breaking change if we simply went ahead with replacing
is_page_heading: true with heading_level: 1 and heading_size: 'xl'
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants