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.

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

Refactor component JavaScript and add tests #538

Merged
merged 36 commits into from
Nov 4, 2019

Conversation

tomdoughty
Copy link
Contributor

@tomdoughty tomdoughty commented Oct 1, 2019

Description

Refactor the component JavaScript to use modern ES6 syntax with new JavaScript coding standards for NHS.UK frontend.

Draft of a custom written details polyfill including toggle class and attribute utils I have separated into their own files.

The exisiting polyfill was overkill for our browser support which caused complications for SPAs. I will be adding unit tests for this as part of the ES6 refactor but thought it is worth creating this PR for discussion. Done this way it is a non breaking change so could be merged if we decide it is better.

Update to the latest version of GOV.UK autocomplete

Related issues

#450 #408 #545

Checklist

@chrimesdev
Copy link
Member

chrimesdev commented Oct 21, 2019

Hey @tomdoughty this looks really good. Do you think we could include it in the release 3.0.0? Would be good to have the breaking changes all go out together rather than do another one in a month or so. We are putting 3.0.0 changes on the develop branch if you want to rebase with that.

We're going to be removing the feedback banner, so any JavaScript relating to that could be deleted.

@chrimesdev
Copy link
Member

chrimesdev commented Oct 21, 2019

These are mainly reminders for myself so I don’t forget, but stuff we could look at:

  • Update the any documentation or examples for importing JavaScript, as the import names have changed eg. nhsuk_header(); to Header(); eg. https://github.com/nhsuk/nhsuk-frontend/blob/master/docs/installation/installing-with-npm.md#option-2-import-javascript-using-modules
  • Can we import and initialise details.polyfill.js the same way we do other components for consistency. eg import Details from './components/details/details.polyfill'; Details();
  • Do we still need to use document.addEventListener('DOMContentLoaded', () => { when initialising the components JavaScript
  • Test the compiled JavaScript (nhsuk.min.js) in a separate app that uses compiled files to consume the FE library
  • Will need a change log entry for the breaking change and how to update applications

Can be done at a later date:

  • Add documentation to the contributing guidelines for Jest tests, prehooks and JavaScript coding standards

This pull request also potentially fixes #450 & #408 & #545

@tomdoughty tomdoughty force-pushed the fix/details-polyfill-replacement branch from 42d183e to 5cf1f10 Compare October 22, 2019 08:09
@chrimesdev chrimesdev changed the base branch from master to develop October 23, 2019 17:45
@chrimesdev chrimesdev force-pushed the fix/details-polyfill-replacement branch from 5002a99 to 47180f9 Compare October 23, 2019 17:46
@chrimesdev chrimesdev changed the title Fix/details polyfill replacement Refactor component JavaScript and add tests Oct 23, 2019
@chrimesdev chrimesdev self-requested a review October 23, 2019 17:50
@chrimesdev
Copy link
Member

Just need to do some final testing with the JS (in a package and compiled versions) and add a change log entry then this will be good to go in release 3.0.0 🍾

@chrimesdev chrimesdev marked this pull request as ready for review October 23, 2019 18:23
@chrimesdev
Copy link
Member

The compiled JavaScript seems to be working fine.

@chrimesdev chrimesdev force-pushed the fix/details-polyfill-replacement branch from 9f7ae9f to 817d908 Compare November 4, 2019 10:03
@chrimesdev
Copy link
Member

Will work on the changelog on develop

@chrimesdev chrimesdev merged commit 944cfeb into develop Nov 4, 2019
@chrimesdev chrimesdev deleted the fix/details-polyfill-replacement branch November 4, 2019 10:40
@chrimesdev
Copy link
Member

Thanks @tomdoughty 🍾 🔥

@chrimesdev chrimesdev mentioned this pull request Nov 4, 2019
3 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants