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

Allow the panel component title heading level to be customisable #853

Merged
merged 2 commits into from
Jul 5, 2018

Conversation

kr8n3r
Copy link

@kr8n3r kr8n3r commented Jul 2, 2018

This modification allows it panel component title to be any heading level by specifying headingLevel parameter
Default is 2.

This PR

  • Adds test
  • Adds an example
  • Updates component table of arguments
  • Generates an updated README

Adresses: alphagov/govuk-design-system#413

@govuk-design-system-ci govuk-design-system-ci temporarily deployed to govuk-frontend-review-pr-853 July 2, 2018 11:24 Inactive
@govuk-design-system-ci govuk-design-system-ci temporarily deployed to govuk-frontend-review-pr-853 July 2, 2018 11:27 Inactive
@@ -1,9 +1,10 @@
{% set titleLevel = params.titleLevel if params.titleLevel else 1 %}
Copy link
Contributor

@NickColley NickColley Jul 2, 2018

Choose a reason for hiding this comment

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

If the default is 1, this will be a breaking change, since before we were defaulting to H2.

Maybe we should default it to 2 and add a deprecation notice?

Copy link
Author

Choose a reason for hiding this comment

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

In most cases it will (should) be an h1 so i guess a breaking change.

Copy link
Author

Choose a reason for hiding this comment

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

filed under Breaking change

Copy link
Contributor

Choose a reason for hiding this comment

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

If we're going to make this a breaking change we should probably hold this for now and batch it up with other breaking changes.

Copy link
Contributor

Choose a reason for hiding this comment

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

We can make do this work and default to 2, then update the examples in the Design System to explicitly set the level as 1.

We'd then deprecate this behaviour and release the default as 1 in a breaking release later on?

Maybe I'm overcomplicating it.

@kr8n3r kr8n3r force-pushed the allow-panel-heading-level-customisation branch from 2739ad0 to 2216612 Compare July 2, 2018 13:41
@govuk-design-system-ci govuk-design-system-ci temporarily deployed to govuk-frontend-review-pr-853 July 2, 2018 13:41 Inactive
@kr8n3r kr8n3r changed the title Allow panel title heading level to be customisable Change Panel component title from h2 to h1 and allow heading level to be customisable Jul 2, 2018
@kr8n3r kr8n3r changed the title Change Panel component title from h2 to h1 and allow heading level to be customisable Change the panel component title from h2 to h1 and allow heading level to be customisable Jul 2, 2018
@kr8n3r kr8n3r changed the title Change the panel component title from h2 to h1 and allow heading level to be customisable Change the panel component title from h2 to h1 and allow the heading level to be customisable Jul 2, 2018
@@ -1,9 +1,10 @@
{% set titleLevel = params.titleLevel if params.titleLevel else 1 %}
Copy link
Contributor

Choose a reason for hiding this comment

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

Would headingLevel be better suited?

Copy link
Author

Choose a reason for hiding this comment

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

thought about that. we have titleText, so 1st thought was titleHeadingLevel to keep naming related, but that is wordy.
I also toyed with the idea to actually move the title to an object, to have:

title: {
 text:
 html: 
 headingLevel: 
}

text: 'no'
},
{
text: 'Optional heading level, from 2 to 6. Default is 1.'
Copy link
Contributor

Choose a reason for hiding this comment

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

From 1 to 6.

@kr8n3r kr8n3r force-pushed the allow-panel-heading-level-customisation branch from 2216612 to f57dd5e Compare July 3, 2018 09:44
@govuk-design-system-ci govuk-design-system-ci temporarily deployed to govuk-frontend-review-pr-853 July 3, 2018 09:44 Inactive
@kr8n3r kr8n3r force-pushed the allow-panel-heading-level-customisation branch from f57dd5e to b7022d8 Compare July 3, 2018 10:50
@govuk-design-system-ci govuk-design-system-ci temporarily deployed to govuk-frontend-review-pr-853 July 3, 2018 10:50 Inactive
@kr8n3r kr8n3r force-pushed the allow-panel-heading-level-customisation branch from b7022d8 to 2c05542 Compare July 3, 2018 22:14
@govuk-design-system-ci govuk-design-system-ci temporarily deployed to govuk-frontend-review-pr-853 July 3, 2018 22:14 Inactive
@kr8n3r kr8n3r force-pushed the allow-panel-heading-level-customisation branch from 2c05542 to f7b9789 Compare July 3, 2018 22:15
@govuk-design-system-ci govuk-design-system-ci temporarily deployed to govuk-frontend-review-pr-853 July 3, 2018 22:15 Inactive
@kr8n3r kr8n3r force-pushed the allow-panel-heading-level-customisation branch from f7b9789 to 4ab37b2 Compare July 3, 2018 22:23
@govuk-design-system-ci govuk-design-system-ci temporarily deployed to govuk-frontend-review-pr-853 July 3, 2018 22:23 Inactive
@kr8n3r kr8n3r changed the title Change the panel component title from h2 to h1 and allow the heading level to be customisable Allow the panel component title heading level to be customisable Jul 3, 2018
@kr8n3r kr8n3r force-pushed the allow-panel-heading-level-customisation branch from 4ab37b2 to 50762ee Compare July 4, 2018 07:46
@govuk-design-system-ci govuk-design-system-ci temporarily deployed to govuk-frontend-review-pr-853 July 4, 2018 07:46 Inactive
@kr8n3r kr8n3r added this to the v1.1.0 milestone Jul 4, 2018
@kr8n3r kr8n3r force-pushed the allow-panel-heading-level-customisation branch from 50762ee to 6360f1c Compare July 4, 2018 12:25
@govuk-design-system-ci govuk-design-system-ci temporarily deployed to govuk-frontend-review-pr-853 July 4, 2018 12:25 Inactive
@kr8n3r kr8n3r force-pushed the allow-panel-heading-level-customisation branch from 6360f1c to cfef1ef Compare July 4, 2018 13:01
@govuk-design-system-ci govuk-design-system-ci temporarily deployed to govuk-frontend-review-pr-853 July 4, 2018 13:01 Inactive
@@ -30,6 +30,22 @@ describe('Panel', () => {
expect(panelTitle).toEqual('Application <strong>not</strong> complete')
})

it('renders title as h1 (as the default heading level)', () => {
Copy link
Contributor

Choose a reason for hiding this comment

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

h2

Copy link
Contributor

@36degrees 36degrees 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, bar one comment which needs updating.

@joelanman
Copy link
Contributor

sorry to comment late, but didn't we want the default to be h1? As that's the common case for this panel? I thought I'd read we were going to do that somewhere?

@kr8n3r
Copy link
Author

kr8n3r commented Jul 5, 2018

@joelanman making it h1 in the next breaking release: https://github.com/alphagov/govuk-frontend/milestone/2

@kr8n3r kr8n3r force-pushed the allow-panel-heading-level-customisation branch from cfef1ef to 866e107 Compare July 5, 2018 18:58
@kr8n3r kr8n3r merged commit e11e5c5 into master Jul 5, 2018
@kr8n3r kr8n3r deleted the allow-panel-heading-level-customisation branch July 5, 2018 19:01
@joelanman
Copy link
Contributor

@igloosi ah cool thanks

@NickColley NickColley mentioned this pull request Jul 13, 2018
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.

5 participants