Remove / refactor some variable declarations #223
Conversation
tvararu
changed the title
|
Added a do not merge label as I'm finding a few more of these. |
| @@ -6,8 +6,6 @@ | |||
| if (typeof GOVUK === 'undefined') { root.GOVUK = {}; } | |||
|
|
|||
| var SelectionButtons = function (elmsOrSelector, opts) { | |||
| var $elms; | |||
There was a problem hiding this comment.
In case it isn't clear, this variable was getting written to, but nobody was reading from it.
There was a problem hiding this comment.
related issue alphagov/govuk_frontend_toolkit#267
There was a problem hiding this comment.
sorry, its not clear but this code doesnt originate in the kit - I think it comes from the Frontend toolkit? Or Elements? @gemmaleigh ?
There was a problem hiding this comment.
Even if that's the case, I'm of the opinion that if it's in source control we should either:
- treat it as our responsibility and fix/maintain it
- remove and declare it as an external dependency (using package.json in this situation)
Let me know what you think. 👌
There was a problem hiding this comment.
yeh you're right - looks like we ought to be able to refer to it in the toolkit that gets downloaded, either with this line or something similar: https://github.com/alphagov/govuk_prototype_kit/blob/master/server.js#L58
There was a problem hiding this comment.
@joelanman: I've just tested and it seems like this behaviour is already implemented, exactly as you describe it. So we can just remove the app/assets/govuk/javascripts/selection-buttons.js dupe from this codebase without any issues.
tvararu
changed the title
|
I think the best way to go ahead with this would be to update the original selection-buttons.js file in the front end toolkit, bump the version of the front end toolkit and then update the version of the front end toolkit we're using for the kit (in package.json) which will then pull in this change. It also means that other repos which consume the JS from the frontend toolkit (for example, GOV.UK elements) can update to get this fix too. There's also another piece of work which involves not committing library code (like this file) to this repo, ideally it should be added to .gitignore and copied over from the frontend_toolkit_npm module when the app is run. |
|
Thanks for the feedback, @gemmaleigh! TODO:
|
A copy of this file already gets imported and served through https://github.com/alphagov/govuk_prototype_kit/blob/master/server.js#L58. As such, we don't need this file. Relevant discusson: #223 (comment)
tvararu
force-pushed
the
remove-unused-variables
branch
from
5caea24 to
a5da682
Compare
|
I've updated this PR and it should be good to merge even without #233, which can be dealt with separately. |
|
Thanks @tvararu 😄 |
Various nits I've found while running a linter through the codebase.