This changeset removes webauthn from the Notify.gov admin app. We are not using webauthn at all in our implementation and will be looking at an entirely different authentication system in the near future.
Signed-off-by: Carlo Costino <carlo.costino@gsa.gov>
- Deleted /stylesheets folder
- Removed sass build from gulpfile
- Changed gov links to usa links
- Changed other govuk styles, like breadcrumbs
- Changed name of uk_components file to us_components
- Fixed a few tests that broke on account of the changes
* Updated header and footer
* Moved files around and updated gulpfile to correct the build process when it goes to production
* Updated fonts
* Adjusted grid templating
* Adding images to assets
* Updated account pages, dashboard, and pages in message sending flow
* Updated the styling for the landing pages in the account section once logged in
We removed whitespace in the HTML of the copy to clipboard component
in https://github.com/alphagov/notifications-admin/pull/4236/files
When the Javascript on the page loads it re-renders the component,
using HTML which is embedded in the .js file.
This means we also need to apply the same change to the .js file
to remove any extraneous whitespace.
In previous iterations of the classPersister, we
found issues with the implementation meant classes
it should have added back to elements were also
added to other elements. This adds tests for this
scenario to ensure it doesn't happen again.
Also includes changes to fix a linting error with
the JS which complained about a function being
defined in a loop while referencing variables in
the outer scope.
The assumption that the classes you want to
persist will always have parity with the elements
that have those classes, at that point, won't
always be true.
Because of that, this changes the way elements
with those classes are stored, to be in a map
between classes and the elements with them (at
that point).
Also includes an extra test for a scenario where
more than one updating component is in the page
with classes that need to persist through updates.
The updateContent JS was changed in this commit so
the replacement of the original HTML (with GOVUK
modules data-attributes) was moved into the start
method rather than being a slightly odd side
effect of the render function diffing:
476ed1593c
This adds a test to make it more clear this
happens, as requested in this comment:
https://github.com/alphagov/notifications-admin/pull/4155#discussion_r804689618
Wrap the code that updates the HTML with changes
from the server with code that stores and
re-applies specified classes.
This is to allow other JS to add classes which
change the visual state of the HTML without them
being considered by the code that diffs our
in-page HTML against that from the server.
They are called classesToPersist because this
should make the visual state they create persist
between updates.
Includes the addition of tests for updateContent
that cover the addition/deletion of elements so we
can write a test for classNames persisting through
updates. The existing tests only cover updates
that change the content of elements. Just adding
the test for these changes to those would simulate
a scenario that doesn't exist in the app. Writing
extra tests for the kind of updates these changes
act on keeps them in line with the app code.
We added domdiff to replace the DiffDOM library
here:
87f54d1e88
DiffDOM had updated its code to be written to the
ECMAScript 6 (ES6) standard and so needed extra
work to work with the older browsers in our
support matrix. This was recorded as an issue
here:
https://www.pivotaltracker.com/n/projects/1443052/stories/165380360
Domdiff didn't work (see below for more details)
so this replaces it with the morphdom library.
Morphdom supports the same browsers as us and is
relied on by a range of large open source
projects:
https://github.com/patrick-steele-idem/morphdom#what-projects-are-using-morphdom
It was tricky to find alternatives to DiffDOM so
if we have to source alternatives in future, other
options could be:
- https://github.com/choojs/nanomorph
- https://diffhtml.org/index.html (using its
outerHTML method)
Why domdiff didn't work
Turns out that domdiff was replacing the page HTML
with the HTML from the AJAX response every time,
not just when they differed. This isn't a bug.
Domdiff is bare bones enough that it compares old
DOM nodes to new DOM nodes with ===. With our
code, this always results to false because our new
nodes are made from HTML strings from AJAX
response so are never the same node as the old
one.
In 674c27a693 we updated the autofocus
Javascript to be compatible with GOV.UK Frontend textboxes, which have
the `data-module` attribute set on the `input` element, rather than on
a wrapper element.
However we still have some `<textarea>`s and `<input>`s which haven’t
moved to GOV.UK Frontend and therefore aren’t getting picked up by the
Javascript which is supposed to focus them.
This commit makes the Javascript work with both kinds of textbox, which
is needed until we move entirely to GOV.UK Frontend.
This is currently spelt incorrectly though it
seemed to work nonetheless. Can only assume this
is a common error, for this attribute or all
attribute names, so browsers work it out.
This makes the spelling match the spec:
https://www.w3.org/TR/wai-aria/#aria-describedby
The way we're using the updateContent.js code is
slightly different to expected and to the
scenarios in our tests. This changes the
tests to match that use.
The expected behaviour was for updates to a
module's HTML to happen to the HTML inside of the
div[data-module=update-content] element.
So with initial HTML of:
<div data-module="update-content" data-key="one">
<div class="ajax-block-container">
Existing content
</div>
</div>
...should be updated to be:
<div data-module="update-content" data-key="one">
<div class="ajax-block-container">
New content
</div>
</div>
Instead the HTML returned by the AJAX requests
replaced the div[data-module=update-content]
element.
So with initial HTML of:
<div data-module="update-content" ..>
<div class="ajax-block-container">
Existing content
</div>
</div>
...will be updated to be:
<div class="ajax-block-container">
New content
</div>
This doesn't seem to create any noticable changes
to the visual interface so, I think, went
unnoticed. The assumption I am making, of this
being unintended, is based on the fact that the
div[data-module=update-content] element has an
aria-live attribute, which authors would normally
want to stay in the page when updates happen.
Note: This commit doesn't try and fix the problem,
as the behaviour still largely works and the lack
of aria-live actually seems to be a positive
thing, meaning non-visual users aren't told of
every update but can discover it themselves if
needed.
A while ago diffDOM moved its code to use ES6
modules and started using various language
features specific to ES6. These two things
happened independently btw.
The result of this is that the version of diffDOM
suitable for our build pipeline, structured as an
immediately invoked function evocation (IIFE),
now requires polyfills of some ES6 features to
work in the older browsers we support, like IE11.
It's also worth noting that in the move to ES6
the maintainers of diffDOM have adopted a process
whereby users who need to support older browsers
now have to add polyfill code for any ES6 features
they choose to use.
This commmit proposes a move to the domdiff
library instead because:
- it runs on all javascript runtimes with no
polyfills
- it is 2KB instead of diffDOM's 25KB
Domdiff takes a different approach to diffDOM, in
that it compares existing nodes and new nodes and
replaces the existing ones with the new ones if
there are differences. By contrast, diffDOM will
make in-place changes to nodes if there are enough
similarities. In other words, in most situations,
diffDOM won't change the node in $component
whereas domdiff will.
Because of this, I've had to change the
updateContent.js code to cache the data-key
attribute's value so we don't lose access to it by
overwrite the $component variable with a different
jQuery selection.
since we are hard-coding a generic error message on the front-end, we
have no need to do anything on the back end. This is also nice as it
standardises the two flows to behave more like each other (rather than
previously where one would `flash` an error message and the other would
return CBOR for the js to decode).
Note that the register flow returns 400 while the auth flow returns 403.
The js for both just checks `response.ok` so will handle both. The JS
completely discards any body returned if the status isn't 200 now.
turns out that we're only using errorBanner with a static message, and
it's also full of rich html content. This means that it's probably
better to put it in the html templates with other content, rather than
hidden away in js files if we can help it.
Since there are two places, had to dupe the error message but i think
that's fine as i don't anticipate this error message being used in
significantly more places.
making it a string is a bit gross and means we don't get nice syntax
highlighting on it, but as it needs to be passed in to a jinja macro
that's the way it has to go unfortunately.
the banner is a nicer user experience, and consistent with how we
display errors elsewhere in notify. For now pass through the error
message from JS, but we'll probably want to change that since the erorr
messages themselves are often a bit cryptic and unhelpful
this ensures it's reusable by other components, and easier to unit test
by isolating the separate concerns
note: this is not in Modules since that's designed for classes that are
then bound to an element in the DOM as indicated by a data-module
attribute. This will just live at the window.GOVUK level since we want
there to only ever be one `.banner-dangerous` warning.
We only need to assert on the URL for the subsequent POST back to
the server, at which point we can call the test "done()". This is
a technique we use in the following tests as well, so we don't need
to comment about it here.
This is easier than re-assigning the mock functions manually, as
we're reusing Jest's in-built behaviour. Because all the mocks
are restored, we need to move the ones we had in the beforeAll
block into the beforeEach block.
Note: "require('./support/teardown.js')" also resets all Jest
mocks, but "require" only runs once, so we can't use it in a
beforeEach block [1]. We could do a "jest.resetModules()" to fix
that, which seems worse on the whole. I think there's a broader
discussion here about whether we could / should have a global
reset of Jest mocks after each test - I quickly tried this and
it causes some existing tests to fail :-|.
[1]: https://stackoverflow.com/questions/48989643/how-to-reset-module-imported-between-tests
