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 tests for govuk-c-warning-text component #479

Merged
merged 2 commits into from
Jan 30, 2018
Merged

Conversation

hannalaakso
Copy link
Member

This PR adds Jest tests for the warning text component.

They test if the component:

  • renders the default example with text and assistive text
  • renders the default example with aria attribute for icon
  • renders classes
  • renders custom text and custom assistive text
  • renders escaped html when passed to text
  • renders html
  • renders attributes

@govuk-design-system-ci govuk-design-system-ci temporarily deployed to govuk-frontend-review-pr-479 January 29, 2018 16:38 Inactive
@govuk-design-system-ci govuk-design-system-ci temporarily deployed to govuk-frontend-review-pr-479 January 29, 2018 16:39 Inactive
@hannalaakso hannalaakso changed the title Add Jest tests for warning text component Add Jest tests for govuk-c-warning-text component Jan 29, 2018
@hannalaakso hannalaakso changed the title Add Jest tests for govuk-c-warning-text component Add tests for govuk-c-warning-text component Jan 29, 2018
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.

Generally looks great. I think there's a couple of places where we could be clearer about what we want to be happening, either by using a clearer description or just making sure we're not testing too much at once.

expect($assistiveText.text()).toEqual('Warning')
})

it('renders the default example with aria attribute for icon', () => {
Copy link
Contributor

Choose a reason for hiding this comment

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

We could be clearer with our intent here. How about something like…

It hides the icon from screen readers using the aria-hidden attribute

?

Copy link
Member Author

Choose a reason for hiding this comment

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

A very good call 💯 Have amended.

expect($component.hasClass('govuk-c-warning-text--custom-class')).toBeTruthy()
})

it('renders custom text and custom assistive text', () => {
Copy link
Contributor

Choose a reason for hiding this comment

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

I think this would make more sense as two different tests.

Copy link
Member Author

Choose a reason for hiding this comment

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

I was in two minds about that but I think you're right. Have split it into two tests.

'data-test': 'attribute',
'id': 'my-warning-text'
},
html: '<span>Some custom warning text</span>'
Copy link
Contributor

Choose a reason for hiding this comment

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

This isn't being tested here. Can it just be omitted?

@govuk-design-system-ci govuk-design-system-ci temporarily deployed to govuk-frontend-review-pr-479 January 30, 2018 09:46 Inactive
@govuk-design-system-ci govuk-design-system-ci temporarily deployed to govuk-frontend-review-pr-479 January 30, 2018 09:52 Inactive
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.

Great stuff 👌

They test if the component:

- renders the default example with text and assistive text
- renders the default example with aria attribute for icon
- renders classes
- renders custom text and custom assistive text
- renders escaped html when passed to text
- renders html
- renders attributes
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