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 character-count script #1055

Merged
merged 1 commit into from
Nov 8, 2018
Merged

Add tests for character-count script #1055

merged 1 commit into from
Nov 8, 2018

Conversation

alex-ju
Copy link
Contributor

@alex-ju alex-ju commented Nov 5, 2018

This PR contains a basic set of tests for the character count component that were mistakenly missed from the initial PR by author's fault.

Issue: #1039

@36degrees 36degrees self-requested a review November 7, 2018 14:22
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.

These look great, thanks Alex. There's a few long lines and minor things that I think could possibly be simplified, but I'd be happy to merge this as it stands – let me know if you do want to make any changes.


// Press 11 characters
await page.focus('.js-character-count')
for (let i = 0; i < 11; i++) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Not blocking: would page.keyboard.type('A'.repeat(11)) have the same effect?

Copy link
Contributor Author

@alex-ju alex-ju Nov 7, 2018

Choose a reason for hiding this comment

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

Repeat doesn't seem to be working, unfortunately, so I'll have to leave the for.

src/components/character-count/character-count.test.js Outdated Show resolved Hide resolved
})

describe('Character count', () => {
describe('when JavaScript is unavailable or fails', () => {
Copy link
Contributor

Choose a reason for hiding this comment

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

👌 Great to see tests for this.

@alex-ju
Copy link
Contributor Author

alex-ju commented Nov 7, 2018

@36degrees thanks. will update with your suggestions. .repeat(11) feels better 😅

@alex-ju
Copy link
Contributor Author

alex-ju commented Nov 7, 2018

@36degrees updated with suggestions except for .repeat() which is not producing the desired effect. happy to try other approaches as I'm not fond of that for loop, but it's doing the job.

Copy link

@kr8n3r kr8n3r left a comment

Choose a reason for hiding this comment

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

Thanks @alex-ju and @36degrees

@kr8n3r kr8n3r added this to the [NEXT] milestone Nov 8, 2018
@kr8n3r kr8n3r merged commit ae8fe5f into alphagov:master Nov 8, 2018
@alex-ju alex-ju deleted the add-character-count-test branch November 8, 2018 09:19
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