-
Notifications
You must be signed in to change notification settings - Fork 327
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 the details component #480
Conversation
const { $ } = render('details', examples.default) | ||
|
||
// Look for the summary element within the details element | ||
const $summary = $('.govuk-c-details__summary', '.govuk-c-details') |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Didn't even know you could do this, I've been doing $('...').find('...')
all this time.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, I think it's called 'context' in jQuery-land. I've seen in the past people generally aren't that aware of it – hence the comment. Do you think it's clear enough what's it doing?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In this specific case I'd just rely on good ol' CSS selectors $('.govuk-c-details .govuk-c-details__summary')
which would hopefully be more widely understood.
Either way the comment is good, so not blocking.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That's a good call, though not sure we'd want to use >
as that would be testing that the summary is a direct descendant - which I don't think we care about?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I stealth removed the direct descendant, but wasn't quick enough 😢
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
😁
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice, looks good to me. 👍
ade51b9
to
55d83fa
Compare
55d83fa
to
0a91bc0
Compare
https://trello.com/c/lJzDNEvk/642-automated-tests-for-details-component