Add details.polyfill.js #426
Conversation
Make details.polyfill.js available to anyone using govuk_frontend_toolkit
|
Useful. 👍 |
|
@alex-ju is there a Jasmine spec to include with this? |
|
Updated
|
spec/unit/details.polyfill.spec.js
Outdated
| this.$content.remove() | ||
| }) | ||
|
|
||
| describe('when details element follows the specified structure', function () { |
There was a problem hiding this comment.
I haven't seen describe blocks used in this nested manner before – I guess it's just trying to describe the context, but I'm not convinced it's required. Most of it seems pretty self explanatory. Are there other benefits to using them that I'm not understanding?
There was a problem hiding this comment.
I'm not used with this way of nesting descriptions, I just followed other tests' structure in the frontend_toolkit. I can flatten them down.
There was a problem hiding this comment.
Aah, sorry – for some reason I didn't even think to look at how the other specs were written. If we're broadly following the conventions of the other specs, let's leave them as they are and re-visit this when we look at moving this into Frontend.
spec/unit/details.polyfill.spec.js
Outdated
|
|
||
| describe('when details element follows the specified structure', function () { | ||
| describe('and when details.polyfill is initialised', function () { | ||
| describe('and when we have native details support', function () { |
There was a problem hiding this comment.
When we don't have native support? Surely if we have native support this doesn't do anything?
spec/unit/details.polyfill.spec.js
Outdated
|
|
||
| it('should make the hidden content visible if its summary is clicked', function (done) { | ||
| // Initialise again | ||
| this.detailsPolyfill = GOVUK.details.addDetailsPolyfill() |
spec/unit/details.polyfill.spec.js
Outdated
| GOVUK.details.started = false | ||
| }) | ||
| it('should add the aria attributes to summary', function () { | ||
| expect(this.$summary1.attr('role')).toBe('button') |
There was a problem hiding this comment.
Can we try and have one expectation per it block? For example 'it should add the button role to the summary', 'it should associate the summary with the content using aria-controls', 'it should indicate the collapsed state using aria-expanded'
spec/unit/details.polyfill.spec.js
Outdated
|
|
||
| // Trigger click on summary | ||
| this.$summary1.click() | ||
| expect(this.$content.attr('open')).toBe('open') |
There was a problem hiding this comment.
It would be good to rework this to have one expectation per it block as well. Perhaps
describe: when the summary is clicked
it: should make the content visible
it: should indicate the expanded state of the summary using aria-expanded
it: should indicate the visible state of the content using aria-hidden
| // Add to page | ||
| $(document.body).append(this.$content) | ||
|
|
||
| setTimeout(function () { |
There was a problem hiding this comment.
Would be good to add a comment as to why this is needed.
There was a problem hiding this comment.
Essentially I needed to do asynchronous testing because of how the events are being chained.
I can avoid doing so, but I have to do more rewriting on the polyfill first. Let me know if you want me to try and get rid of the async test.
- Flatten the nested describe blocks - Test one expectation per ‘it’ block - Remove multiple initialisation
spec/unit/details.polyfill.spec.js
Outdated
| this.$content.remove() | ||
| }) | ||
|
|
||
| describe('and when details.polyfill is initialised', function () { |
There was a problem hiding this comment.
Suggest just "When the polyfill is initialised" – there's no real prior statement to connect the 'and' to.
spec/unit/details.polyfill.spec.js
Outdated
| expect(this.$hiddenContent1.attr('id')).toBe('details-content-0') | ||
| }) | ||
|
|
||
| it('should set aria-hidden true to hidden content', function () { |
There was a problem hiding this comment.
This doesn't scan very well to me. I'd suggest something like 'should present the content as hidden using aria-hidden'
Promote the details polyfill to Frontend Toolkit from GOV.UK Elements – PR #426
Make details.polyfill.js available to anyone using govuk_frontend_toolkit; fixes Issue #400.