Allow Button to receive configuration via JavaScript #2867

romaricpascal · 2022-09-20T15:57:20Z

The PRs for adding configuration via JavaScript carries on with the Button. 🎉

Changes are similar to the ErrorSummary and NotificationBanner and the commits paint a step-by-step picture of the changes.

The most notable thing is that it slightly changes when the value for whether to prevent double click is read. On main the value is read within the click listener but with this update, it'll be read at component instantiation. This has an effect on the following scenario:

the attribute could be set when the component is instantiated
some other piece of JS on the page may remove it while the user browses
at the time the use click, the attribute would then not be there, calling for a different behaviour than when the component was initialised

On main the Button will behave according to the value of the attribute at step 3, while after this PR it'll behave according to the value at step 1.

Consistency with the other components has been prioritised here after discussing that updating the data-attribute at runtime was not an expected nor supported use case.

Closes #2836.

- Create helper functions for tracking the clicks - Group tests ahead of adding JavaScript config ones

This'll be necessary to ensure proper access to data-attribute configuration, in case consuming code initialises the component on their own without checking that the element they pass is an actual element (for ex. passing through the result of `document.querySelector` on a page without the targeted element)

querkmachine · 2022-09-21T13:58:17Z

src/govuk/components/button/button.mjs

 }

 /**
 * Initialise component
 */
 Button.prototype.init = function () {
+  if (!this.$module) {


Asking for clarification:

At the start of the module we're checking for the existence of $module, and only if it exists does the code to assign $module to this.$module run.

Here we are then checking for the existence of this.$module, however wouldn't the previous steps ensure that this statement would never evaluate to false?

Unfortunately we need that extra check in init because the first check only shortcuts the constructor, to safeguard the access ot the module's dataset. The addEventListener calls happen inside init() fail if the component was instantiated with an undefined/null element.

If the constructor was called with an undefined element, wouldn't we want to immediately halt execution anyway?

I don't think there's anything we can do from the constructor to stop the init function from being called – that's happening outside of the object (new govukButton($module).init()).

Do you mean letting the JS throw an Error when trying to access $module.dataset in the constructor?

We had to set a safety for that in the ErrorSummary to avoid making a breaking change for users that may have a mis-initialised ErrorSummary component. For ex. with a blanket new ErrorSummary(document.querySelector('[data-module="error-summary"]')) that runs on a page without an error summary. With the current version, their code wouldn't break, so we wanted to keep that happening after we added the configuration. I'd be quite keen to remove that safety in the next breaking version as it'll be more informative to users to have an error in their console.

For consistency, I've set that same mechanism on the Button, but need the checks in the two places because of the two step initialisation of components (instantiation + init()).

Sorry Ollie, I was commenting on an old version of the page and hadn't seen you'd replied.

I don't think there's anything we can do from the constructor to stop the init function from being called – that's happening outside of the object (new govukButton($module).init()).

We could always make init() a no-op in the constructor. That would keep everything in the same place. That'd actually make the thing quite portable from component to component.

if (!$module) { // Make `init` a no-op to avoid potentially breaking code at initialisation // from lack of HTML element this.init = function(){} return this }

I'll actually update the PR with that approach as:

it'll avoid the awkward second check that prompted @querkmachine's question

it'll be easy to pass from one component to another

I'll open a second PR to amend the components we've already made configurable.

I'm not 100% sure about this from a code readability perspective – appreciate at the minute the init function is near the constructor but I think it'd be easy to miss and I don't think I'd expect a class method to be redefined in this way…

If I was writing this I think I'd stick with the two guard clauses we had before, especially if we plan to get rid of the init function in the short-to-mid term.

Happy to reset to the previous commit, it's not a solution I was particularly attached to and it'll become irrelevant when we remove the init function. Just thought it'd make sense to keep everything in a single place instead of the double check, especially as it's for handling an "error" scenario.

36degrees

This is looking good – just a few minor formatting things that need tidying up

36degrees · 2022-09-26T08:54:27Z

src/govuk/components/button/button.test.js

+       * Wraps the button rendered on the page in a form
+       *
+       * Examples don't do this and we need it to have something to submit
+       */


Indentation

36degrees · 2022-09-26T08:54:38Z

src/govuk/components/button/button.test.js

-          })
+  describe('preventing double clicks', () => {
+    // Click counting is done through using the button to submit
+    // a form and counting sumbissions. It requires some bits of recurring


Suggested change

// a form and counting sumbissions. It requires some bits of recurring

// a form and counting submissions. It requires some bits of recurring

36degrees · 2022-09-26T08:55:26Z

src/govuk/components/button/button.test.js

+       * Gets the number of times the form was submitted
+       *
+       * @returns {Number}
+       */


Indentation

36degrees · 2022-09-26T09:15:14Z

src/govuk/components/button/button.test.js

-            // Don't refresh the page, which will destroy the context to test against.
-            event.preventDefault()
-          })
+          const $buttonPrime = $button.cloneNode(true)


It took me a while to work out what this meant (and I'm still not 100% sure – guessing this is Is prime as in 'a derivative'?)

Wondering if $secondButton might be simpler and clearer?

36degrees

👍🏻

govuk-design-system-ci temporarily deployed to govuk-frontend-pr-2867 September 20, 2022 15:57 Inactive

romaricpascal added 6 commits September 20, 2022 17:05

Refactor button test

812ad37

- Create helper functions for tracking the clicks - Group tests ahead of adding JavaScript config ones

Allow button to receive configuration via JavaScript

8d5577a

Allow setting Button config through initAll

86292d7

Render data-prevent-double-click explicitely in Nunjucks template

413248d

Test that data attributes take precendence over JS config

01d2304

romaricpascal force-pushed the button-config branch from 3cb274a to 01d2304 Compare September 20, 2022 16:06

govuk-design-system-ci temporarily deployed to govuk-frontend-pr-2867 September 20, 2022 16:06 Inactive

querkmachine reviewed Sep 21, 2022

View reviewed changes

govuk-design-system-ci temporarily deployed to govuk-frontend-pr-2867 September 22, 2022 15:41 Inactive

romaricpascal force-pushed the button-config branch from eb59149 to 01d2304 Compare September 26, 2022 08:27

govuk-design-system-ci temporarily deployed to govuk-frontend-pr-2867 September 26, 2022 08:27 Inactive

romaricpascal requested review from querkmachine and 36degrees September 26, 2022 08:39

36degrees reviewed Sep 26, 2022

View reviewed changes

Fix typos and poor naming

a9c6143

govuk-design-system-ci temporarily deployed to govuk-frontend-pr-2867 September 26, 2022 10:08 Inactive

36degrees approved these changes Sep 26, 2022

View reviewed changes

romaricpascal merged commit a28e6ec into main Sep 26, 2022

romaricpascal deleted the button-config branch September 26, 2022 10:29

romaricpascal mentioned this pull request Nov 10, 2022

Release v4.4.0 #3000

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Allow Button to receive configuration via JavaScript #2867

Allow Button to receive configuration via JavaScript #2867

romaricpascal commented Sep 20, 2022

querkmachine Sep 21, 2022

romaricpascal Sep 22, 2022

querkmachine Sep 22, 2022

36degrees Sep 22, 2022

romaricpascal Sep 22, 2022

romaricpascal Sep 22, 2022

romaricpascal Sep 22, 2022 •

edited

Loading

36degrees Sep 22, 2022

romaricpascal Sep 22, 2022

36degrees left a comment

36degrees Sep 26, 2022

36degrees Sep 26, 2022

36degrees Sep 26, 2022

36degrees Sep 26, 2022

36degrees left a comment

	// a form and counting sumbissions. It requires some bits of recurring
	// a form and counting submissions. It requires some bits of recurring

Allow Button to receive configuration via JavaScript #2867

Allow Button to receive configuration via JavaScript #2867

Conversation

romaricpascal commented Sep 20, 2022

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

romaricpascal Sep 22, 2022 • edited Loading

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

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

romaricpascal Sep 22, 2022 •

edited

Loading