Remove jQuery from browser extension components

alphagov / govuk-browser-extension

Chrome and Firefox extension for developers working on GOV.UK.

22 stars 5 forks source link

Remove jQuery from browser extension components #176

Closed kashifatcha closed 9 months ago

kashifatcha commented 11 months ago

What

Going through the codebase and finding where jQuery has been used in the browser extension's components and replacing it with vanilla JavaScript. The tests also included jQuery and needed to be migrated over as well.

Why

As we are migrating the manifest version of the browser extension from 2 to 3, one of the requirements, it would have been difficult to do this whilst the code was still written in jQuery. Also, as jQuery had been removed from most of Gov.uk, this was a good opportunity to remove it from the extension as well. This was the first time that we had worked on the repo, and it allowed us to better understand the codebase and how it works.

https://trello.com/c/3KgjI5Cr/2178-remove-jquery

KludgeKML commented 11 months ago

Looks okay to me, if you address Jon's comments.

KludgeKML commented 11 months ago

@jon-kirwan is there a coverage tool that works with Jasmine? (ie can @kashifatcha see what code coverage this version has compared to the old version)

jon-kirwan commented 11 months ago

@jon-kirwan is there a coverage tool that works with Jasmine? (ie can @kashifatcha see what code coverage this version has compared to the old version)

@KludgeKML interesting question. I don't know much but a quick search around seems to suggest that Istanbul is one of the more popular tools for measuring JavaScript code coverage and can be integrated with Jasmine.

There's an open issue on the Design System repo which mentions it too: expand code coverage collection .

Might be a good one to add to the backlog to explore further.

kashifatcha commented 9 months ago

I have made the changes suggested in the review. I will create a separate PR for adding the linter.

jon-kirwan commented 9 months ago

I have made the changes suggested in the review. I will create a separate PR for adding the linter.

Thanks Kash. The changes look great and all work fine for me. I just have two final commit history related suggestions:

you might be able to squash some commits together e.g. you can probably remove the commit for Fix staleReferenceElementError in design-mode-component spec file (that can probably be squashed into one for rewriting the spec files in JavaScript)
add a separate commit for the change to manifest.json, to update the version. Also, in the same commit, run npm run build to create a new package with the changes. More info: https://github.com/alphagov/govuk-browser-extension?tab=readme-ov-file#releasing-the-extension.

If you don't mind taking a look at these changes i'll then hit approve.

kashifatcha commented 9 months ago

I have squashed the commits, so this PR should be ready to go. I will submit a separate PR for the linter and for upgrading to manifest V3.