Allow initAll to be scoped to a specific part of a page #1216

alex-ju · 2019-02-21T16:15:48Z

We currently initialise component at the document level, but there are cases when components are being injected after the document is loaded. I'm proposing to allow initAll function to be scoped, so it can be called for a specific container or part of the page.

Note
We can replace scope with any name that makes more sense.

NickColley · 2019-02-21T16:19:41Z

One thing to think about is we have had users report errors when running initAll twice, and we thought about solving that a different way by making it safe to call it multiple times:

#1127

Doesn't necessarily block this, but I think if we solved that other issue it'd be fine to run initAll whenever new markup is injected into the page.

It's also worth noting that custom elements (web components) avoid this issue, and are enhanced whenever the browser detects the custom element.

If we do go with this approach, we'd need to document this scoping feature and have tests for it.

hannalaakso · 2019-02-28T11:23:24Z

I've chatted with @alex-ju about this. It seems that there might be value in building in both approaches:

Do a check to see if the component is already initialised (Investigate components being initialised more than once #1127 and what @NickColley drafted)
Initialising against a certain scope

The main value with 2. is initialising based on a scope with DOM elements that are added after document.load to avoid querying the whole DOM (this is linked to #1215).

NickColley · 2019-03-04T10:40:46Z

Agree it probably makes sense to do both, however...

avoid querying the whole DOM

Since the user will be running 'initAll' themselves I don't think there's much overhead, I'd understand if we were listening to DOM changes with mutation observers maybe...

NickColley · 2019-03-05T15:33:01Z

I've added done the following:

Made the JavaScript work without ES2015 features
Add a test page that demonstrates the scoping and corresponding puppeteer tests.
Added some documentation to the installation instructions to explain how to use this feature.

alex-ju · 2019-03-05T16:24:53Z

Thanks for taking the time to properly update this PR @NickColley. This is very much aligned with the need for a scoped initialisation.

NickColley

Needs a CHANGELOG entry

alex-ju · 2019-03-06T14:07:57Z

@NickColley changelog entry added. thanks a lot for the test and doc!

Changelog has been added

NickColley · 2019-03-06T16:36:19Z

OK I think this is ready for review

36degrees · 2019-03-08T10:51:19Z

docs/installation/installing-with-npm.md

@@ -154,6 +154,21 @@ Including the script elsewhere will stop components from functioning or displayi
 </html>
 ```

+#### Initialise GOV.UK Frontend in only certain sections of a page


❤️ 📖

@soupdragon99 has helped on this one

36degrees · 2019-03-08T10:54:36Z

src/all.test.js

+      // 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', { visible: false })


We use a combination of visible: false, visible: true and hidden: true in this test – is there a difference between them, or are they just opposites of each other?

It'd be good to be consistent with either the key (visible: true, visible: false) or the value (visible: true, hidden: true) if so.

There's some nuance to it, you can have something in the DOM but not visible, or something not in the DOM and therefore not visible I believe.

I'll try and clarify this.

36degrees · 2019-03-08T10:55:45Z

src/all.test.js

+      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 })


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.

It was indeed testing it when I remove the scope functionality and ran these tests.

36degrees · 2019-03-08T10:59:50Z

src/all.test.js

@@ -90,6 +90,28 @@ describe('GOV.UK Frontend', () => {
        }, component)
      })
    })
+    it('can be initialised scoped to certain sections of the page', async () => {


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 DomElement as the scope and asserting that querySelectorAll is called, and/or stubbing document and asserting that it doesn't get called?

(Just worth thinking about, this isn't a blocking comment 👍)

We are somewhat bound by the limitations of our testing setup.

I'll see if I can stub an element like you've suggested...

Totally fair, don't spend too much time on it 👍

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...

Let me know if you think the current approach is too risky

soupdragon99 · 2019-03-07T12:11:18Z

CHANGELOG.md

@@ -14,10 +14,29 @@

 🆕 New features:

+- Enable GOV.UK Frontend components to be scoped the a specific container


Should this read 'scoped to a specific container' or 'scoped to the specific container' at the moment it's not clear.

36degrees

🔭

NickColley · 2019-03-08T16:15:36Z

@alex-ju thanks for your patience with these pull requests, sorry I did not fully understand your issue the first time.

Let us know when you need us to release these and we'll get it out for you. 👍

alex-ju · 2019-03-12T09:03:36Z

Thanks a lot for pairing on it @NickColley! Would be great to have it in the next planned release.

NickColley added the awaiting triage Needs triaging by team label Feb 21, 2019

alex-ju mentioned this pull request Feb 22, 2019

Support upload and display of new images alphagov/content-publisher#800

Merged

kellylee-gds added submitted-by-user ⚠️ high priority Used by the team when triaging and removed awaiting triage Needs triaging by team labels Feb 27, 2019

kellylee-gds assigned soupdragon99 Feb 27, 2019

kellylee-gds added this to Needs review in Design System Sprint Board via automation Feb 27, 2019

kellylee-gds moved this from Needs review to In progress in Design System Sprint Board Feb 27, 2019

kellylee-gds moved this from In progress to Icebox in Design System Sprint Board Feb 27, 2019

timpaul moved this from Icebox to Backlog in Design System Sprint Board Mar 4, 2019

NickColley removed the submitted-by-user label Mar 5, 2019

NickColley previously requested changes Mar 6, 2019

View reviewed changes

Design System Sprint Board automation moved this from Backlog to In progress Mar 6, 2019

NickColley added this to the [NEXT] milestone Mar 6, 2019

NickColley moved this from In progress to Needs review in Design System Sprint Board Mar 6, 2019

Design System Sprint Board automation moved this from Needs review to In progress Mar 6, 2019

kellylee-gds moved this from In progress to Needs review in Design System Sprint Board Mar 7, 2019

alphagov deleted a comment from alex-ju Mar 8, 2019

36degrees reviewed Mar 8, 2019

View reviewed changes

soupdragon99 reviewed Mar 8, 2019

View reviewed changes

NickColley requested a review from 36degrees March 8, 2019 15:27

alex-ju and others added 4 commits March 8, 2019 16:04

Allow initAll to be scoped to a specific part of a page

e51e9a8

Test that initAll can be called in certain scopes

189ec64

Add documentation explaining the options that can be passed to initAll

6fbfc4a

Add changelog entry

586e294

36degrees approved these changes Mar 8, 2019

View reviewed changes

Design System Sprint Board automation moved this from Needs review to Ready to merge Mar 8, 2019

NickColley merged commit 74f6d5a into alphagov:master Mar 8, 2019

Design System Sprint Board automation moved this from Ready to merge to Done Mar 8, 2019

alex-ju deleted the allow-scoped-init branch March 12, 2019 09:04

NickColley mentioned this pull request Mar 18, 2019

Release v2.9.0 #1249

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Allow initAll to be scoped to a specific part of a page #1216

Allow initAll to be scoped to a specific part of a page #1216

alex-ju commented Feb 21, 2019

NickColley commented Feb 21, 2019 •

edited

hannalaakso commented Feb 28, 2019 •

edited

NickColley commented Mar 4, 2019 •

edited

NickColley commented Mar 5, 2019 •

edited

alex-ju commented Mar 5, 2019

NickColley left a comment

alex-ju commented Mar 6, 2019

NickColley commented Mar 6, 2019

36degrees Mar 8, 2019

NickColley Mar 8, 2019

36degrees Mar 8, 2019

NickColley Mar 8, 2019

36degrees Mar 8, 2019

NickColley Mar 8, 2019

36degrees Mar 8, 2019

NickColley Mar 8, 2019

36degrees Mar 8, 2019

NickColley Mar 8, 2019

NickColley Mar 8, 2019

soupdragon99 Mar 7, 2019

36degrees left a comment

NickColley commented Mar 8, 2019

alex-ju commented Mar 12, 2019

		@@ -14,10 +14,29 @@

		🆕 New features:

		- Enable GOV.UK Frontend components to be scoped the a specific container

Allow initAll to be scoped to a specific part of a page #1216

Allow initAll to be scoped to a specific part of a page #1216

Conversation

alex-ju commented Feb 21, 2019

NickColley commented Feb 21, 2019 • edited

hannalaakso commented Feb 28, 2019 • edited

NickColley commented Mar 4, 2019 • edited

NickColley commented Mar 5, 2019 • edited

alex-ju commented Mar 5, 2019

NickColley left a comment

Choose a reason for hiding this comment

alex-ju commented Mar 6, 2019

NickColley commented Mar 6, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

36degrees left a comment

Choose a reason for hiding this comment

NickColley commented Mar 8, 2019

alex-ju commented Mar 12, 2019

NickColley commented Feb 21, 2019 •

edited

hannalaakso commented Feb 28, 2019 •

edited

NickColley commented Mar 4, 2019 •

edited

NickColley commented Mar 5, 2019 •

edited