Skip to content
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.

Sign up for GitHub

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

Jump to bottom

Remove dependency on jQuery #134

Open
gregtyler opened this issue Jan 8, 2021 · 6 comments
Open

Remove dependency on jQuery #134

gregtyler opened this issue Jan 8, 2021 · 6 comments

Comments

@gregtyler
Copy link
Contributor

gregtyler commented Jan 8, 2021 •
edited

Summary

It would be nice to eliminate our dependency on jQuery, instead using native browser functionality (e.g. setAttribute instead of attr, querySelector rather than find).

Motivation

There are a few reasons I think this is beneficial:

  • It removes a dependency
  • None of the jQuery functionality we use isn't achievable in a (fairly) modern browser
  • It removes a barrier to entry for contributors: they wouldn't need to know jQuery
  • The code is more aligned to current best practices, documentation and tutorials

Describe alternatives you've considered

We can just keep using jQuery and accept the concerns above for now.

Additional context

We'll need to add polyfills for functionality that isn't available in all browsers > IE8. (e.g. Opera Mini and IE9/10 don't support HTMLElement.dataset) We may be able to detect and polyfill automatically as part of the build process.

We may also need to bulk out the IE8 polyfill if that's not already done by the GOV.UK Design system polyfill.

We shouldn't attempt this without unit tests (#132) as it would be difficult to spot regressions.
gregtyler added a commit to ministryofjustice/opg-sirius-lpa-dashboard that referenced this issue Jun 8, 2021
@gregtyler
Add jQuery and expose on window
e2ab95f 
`MOJFrontend` requires this to initialise the Multi select component

There's [an issue in moj-frontend](ministryofjustice/moj-frontend#134) to remove the jQuery dependency, but it's dependent on unit tests
gregtyler added a commit to ministryofjustice/opg-sirius-lpa-dashboard that referenced this issue Jun 14, 2021
@gregtyler
Add jQuery and expose on window
f1cfa40 
`MOJFrontend` requires this to initialise the Multi select component

There's [an issue in moj-frontend](ministryofjustice/moj-frontend#134) to remove the jQuery dependency, but it's dependent on unit tests
gregtyler added a commit to ministryofjustice/opg-sirius-lpa-dashboard that referenced this issue Jun 15, 2021
@gregtyler @dependabot @hawx
VEGA-834: Manager access to caseworker dashboard (#37)
5ccdeda 
* Add `User()` endpoint to sirius package

To get user details by ID

* Add userPendingCases handler

To show a user's pending cases, based on their ID

* Link to user's pending cases from team dashboard

* Add userPendingCases cypress tests

And rename existing file

* Add jQuery and expose on `window`

`MOJFrontend` requires this to initialise the Multi select component

There's [an issue in moj-frontend](ministryofjustice/moj-frontend#134) to remove the jQuery dependency, but it's dependent on unit tests

* Restrict userPendingCases to Managers

Other users shouldn't be able to see each others' dashboards

* Add all caseworker info pages

Adds all cases and tasks pages, with inter-linking and tests

* Remove unused CSS classes

* Expect API user details to include a team

* Add back links to team

* Re-enable codecov-action

They claim to have fixed the issue now

* Add `User()` endpoint to sirius package

To get user details by ID

* Add userPendingCases handler

To show a user's pending cases, based on their ID

* Link to user's pending cases from team dashboard

* Add userPendingCases cypress tests

And rename existing file

* Add jQuery and expose on `window`

`MOJFrontend` requires this to initialise the Multi select component

There's [an issue in moj-frontend](ministryofjustice/moj-frontend#134) to remove the jQuery dependency, but it's dependent on unit tests

* Restrict userPendingCases to Managers

Other users shouldn't be able to see each others' dashboards

* Bump cypress from 7.4.0 to 7.5.0 (#35)

Bumps [cypress](https://github.com/cypress-io/cypress) from 7.4.0 to 7.5.0.
- [Release notes](https://github.com/cypress-io/cypress/releases)
- [Changelog](https://github.com/cypress-io/cypress/blob/develop/.releaserc.base.js)
- [Commits](cypress-io/cypress@v7.4.0...v7.5.0)

---
updated-dependencies:
- dependency-name: cypress
  dependency-type: direct:production
  update-type: version-update:semver-minor
...

Signed-off-by: dependabot[bot] <support@github.com>

Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>

* VEGA-867 Sort pending cases by workedDate (#44)

* Use CanRequestCase on all caseworker pages (#43)

Wherever the "Request next cases" button appears, only show it to users with the "Self Allocation User" role

* Show "Worked, Pending" status (#45)

* Add status-tag template

* Use status-tag template

* Set "Worked, Pending" text explicitly

* Use status-tag template

So that we see "Worked, Pending" everywhere

Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>
Co-authored-by: Joshua Hawxwell <m@hawx.me>
@DanCorder
Copy link

If this is going to take some time (it doesn't sound trivial) then in the meantime it would be nice to make the dependency on jQuery explicit. (I can't see it mentioned in the docs or the package.json)

@gregtyler
Copy link
Contributor Author

gregtyler commented Jun 16, 2021 •
edited

Good point @DanCorder, we should add it to the installing with NPM documentation.

Edit: this has now been added in #187

gregtyler added a commit that referenced this issue Jun 25, 2021
@gregtyler
docs: document jQuery requirement
1e59e62 
As noted in #134, moj-frontend requires JavaScript but never mentions this in documentation.

This change updates documentation to clarify that jQuery must be installed and included.
@gregtyler gregtyler mentioned this issue Jun 25, 2021
Merged
gregtyler added a commit that referenced this issue Jun 25, 2021
@gregtyler
docs: document jQuery requirement
de4d5d6 
As noted in #134, moj-frontend requires JavaScript but never mentions this in documentation.

This change updates documentation to clarify that jQuery must be installed and included.

Also moves the "Installing via NPM" and "Supporting IE8" into the main application
gregtyler added a commit that referenced this issue Jun 26, 2021
@gregtyler
docs: document jQuery requirement (#187)
d7f456c 
As noted in #134, moj-frontend requires JavaScript but never mentions this in documentation.

This change updates documentation to clarify that jQuery must be installed and included.

Also moves the "Installing via NPM" and "Supporting IE8" into the main application
@gregtyler
Copy link
Contributor Author

GDS are considering removing jQuery from the prototype kit, which would mean that (until we remove jQuery ourselves) we'd need to bundle jQuery in the package and add it to the extension config file.

@smcveigh941 smcveigh941 mentioned this issue Mar 12, 2023
Merged
@yndajas
Copy link

yndajas commented Jan 30, 2024 •
edited

Does this need adding to the package.json file until the dependency is removed? I tried removing jQuery per ministryofjustice/hmpps-template-typescript@eec17bb but then found that the sortable tables from this repo need it

@gregtyler
Copy link
Contributor Author

jQuery is listed as a peer dependency in package.json so, from what I understand of peer dependencies, your package manager should either auto-install it (this is what npm seems to do) or warn you to install it manually.

@yndajas yndajas mentioned this issue Jan 30, 2024
Merged
@yndajas
Copy link

yndajas commented Jan 30, 2024

your package manager should either auto-install it (this is what npm seems to do) or warn you to install it manually.

As far as I can tell it didn't do that. npm install output after removing it as a direct dependency on our app but retaining the @ministryofjustice/frontend dependency:

up to date, audited 1177 packages in 785ms

191 packages are looking for funding
  run `npm fund` for details

found 0 vulnerabilities

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

No branches or pull requests
3 participants
@gregtyler @DanCorder @yndajas