-
Notifications
You must be signed in to change notification settings - Fork 320
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 initAll to be scoped to a specific part of a page #1216
Changes from all commits
e51e9a8
189ec64
6fbfc4a
586e294
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,63 @@ | ||
{% extends "template.njk" %} | ||
|
||
{% from "checkboxes/macro.njk" import govukCheckboxes %} | ||
|
||
{% block head %} | ||
<!--[if !IE 8]><!--> | ||
<link rel="stylesheet" href="/public/app.css"> | ||
<!--<![endif]--> | ||
<!--[if IE 8]> | ||
<link rel="stylesheet" href="/public/app-ie8.css"> | ||
<![endif]--> | ||
<!--[if lt IE 9]> | ||
<script src="/vendor/html5-shiv/html5shiv.js"></script> | ||
<![endif]--> | ||
{% endblock %} | ||
|
||
{% block content %} | ||
<h1 class="govuk-heading-xl">Scoped initialisation</h1> | ||
<h2 class="govuk-heading-m">Unscoped section</h2> | ||
<p class="govuk-body">This example should intentionally not have JavaScript functionality</p> | ||
|
||
{{ govukCheckboxes({ | ||
idPrefix: "not-scoped", | ||
name: "not-scoped", | ||
items: [ | ||
{ | ||
value: "not-scoped", | ||
text: "Not scoped", | ||
conditional: { | ||
html: '<p class="govuk-body">Revealed not scoped text</p>' | ||
} | ||
} | ||
] | ||
}) }} | ||
|
||
<h2 class="govuk-heading-m">Scoped section</h2> | ||
<p class="govuk-body">Only this example should have JavaScript functionality</p> | ||
<div id="scoped"> | ||
{{ govukCheckboxes({ | ||
idPrefix: "scoped", | ||
name: "scoped", | ||
items: [ | ||
{ | ||
value: "scoped", | ||
text: "Scoped", | ||
conditional: { | ||
html: '<p class="govuk-body">Revealed scoped text</p>' | ||
} | ||
} | ||
] | ||
}) }} | ||
</div> | ||
{% endblock %} | ||
|
||
{% block bodyEnd %} | ||
<script src="/public/all.js"></script> | ||
<script> | ||
var $scope = document.getElementById('scoped') | ||
window.GOVUKFrontend.initAll({ | ||
scope: $scope | ||
}) | ||
</script> | ||
{% endblock %} |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -90,6 +90,28 @@ describe('GOV.UK Frontend', () => { | |
}, component) | ||
}) | ||
}) | ||
it('can be initialised scoped to certain sections of the page', async () => { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This makes sense, but only tests the instantiation of a single component and feels like it could be relatively fragile if we e.g. changed the way that conditional reveals work. Wondering if there's a better way to test this, maybe as a unit test rather than as an integration test, e.g. passing a stubbed (Just worth thinking about, this isn't a blocking comment 👍) There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. We are somewhat bound by the limitations of our testing setup. I'll see if I can stub an element like you've suggested... There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Totally fair, don't spend too much time on it 👍 There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yeah I think we'd have to run the test using JSDOM rather than in a browser, and it's not clear that you can spy on an element so I'll try to make the current setup clearer but not sure it's worth the time spent... There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Let me know if you think the current approach is too risky |
||
await page.goto(baseUrl + '/examples/scoped-initialisation', { waitUntil: 'load' }) | ||
|
||
// To test that certain parts of the page are scoped we have two similar components | ||
// that we can interact with to check if they're interactive. | ||
|
||
// Check that the conditional reveal component has a conditional section that would open if enhanced. | ||
await page.waitForSelector('#conditional-not-scoped-1', { hidden: true }) | ||
|
||
await page.click('[for="not-scoped-1"]') | ||
|
||
// Check that when it is clicked that nothing opens, which shows that it has not been enhanced. | ||
await page.waitForSelector('#conditional-not-scoped-1', { hidden: true }) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Does this definitely test what we want? It's 'waiting' for something was hidden to still be hidden (which it already was…), rather that asserting that it never becomes visible. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. It was indeed testing it when I remove the scope functionality and ran these tests. |
||
|
||
// Check the other conditional reveal which has been enhanced based on it's scope. | ||
await page.waitForSelector('#conditional-scoped-1', { hidden: true }) | ||
|
||
await page.click('[for="scoped-1"]') | ||
|
||
// Check that it has opened as expected. | ||
await page.waitForSelector('#conditional-scoped-1', { hidden: false }) | ||
}) | ||
}) | ||
describe('global styles', async () => { | ||
it('are disabled by default', async () => { | ||
|
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.
@soupdragon99 has helped on this one