-
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 Button to receive configuration via JavaScript #2867
Merged
Merged
Changes from all commits
Commits
Show all changes
7 commits
Select commit
Hold shift + click to select a range
812ad37
Refactor button test
romaricpascal 583c7fe
Safeguard Button from instantiation without element
romaricpascal 8d5577a
Allow button to receive configuration via JavaScript
romaricpascal 86292d7
Allow setting Button config through `initAll`
romaricpascal 413248d
Render data-prevent-double-click explicitely in Nunjucks template
romaricpascal 01d2304
Test that data attributes take precendence over JS config
romaricpascal a9c6143
Fix typos and poor naming
romaricpascal File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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.
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
tothis.$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?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.
Unfortunately we need that extra check in
init
because the first check only shortcuts the constructor, to safeguard the access ot the module'sdataset
. TheaddEventListener
calls happen insideinit()
fail if the component was instantiated with anundefined
/null
element.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.
If the constructor was called with an
undefined
element, wouldn't we want to immediately halt execution anyway?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 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()
).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.
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-initialisedErrorSummary
component. For ex. with a blanketnew 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()
).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.
Sorry Ollie, I was commenting on an old version of the page and hadn't seen you'd replied.
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.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'll actually update the PR with that approach as:
I'll open a second PR to amend the components we've already made configurable.
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'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.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.
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.