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

Bump paste govspeak to v0.3 #2551

Merged
merged 1 commit into from Jul 27, 2022
Merged

Bump paste govspeak to v0.3 #2551

merged 1 commit into from Jul 27, 2022

Conversation

DilwoarH
Copy link
Contributor

What

Currently, the Paste to Govspeak converter (paste-html-to-govspeak) in the publishing applications automatically converts any H4s, H5s and H6s to a H3 - see https://github.com/alphagov/paste-html-to-govspeak/blob/main/src/html-to-govspeak.js#L88:L89.

We want to remove this functionality and allow H4s, H5s and H6s without them being converted to H3s.

Why

Although GOV.UK doesn't style H4s and below (in non-HTML attachments), it's important for users using assistive technology get the correct heading structure.

https://trello.com/c/aWGmHS2s/573-allow-h4s-to-h6s-with-paste-to-govspeak-converter

⚠️ This repo is Continuously Deployed: make sure you follow the guidance ⚠️

@kevindew
Copy link
Member

The audit commit here looks quite eventful. Lots of version changes going on. The failing test seems to be that we lose the ability to do forEach on a HTMLOptionsCollection

$select.options.forEach(function ($el) {

I guess that might be because CoreJS changed version? did it incorrectly polyfill that method? Are our other uses of forEach safe?

This change:

document.querySelector('select').options
> HTMLOptionsCollection(2) [option, option, selectedIndex: 0]
document.querySelector('select').options.forEach
> undefined

Before:

document.querySelector('select').options
> HTMLOptionsCollection(1155) [option, option, option, option, option, option, option, option, option, option, option, option, option, option, option, option, option, option, option, option, option, option, option, option, option, option, option, option, option, option, option, option, option, option, option, option, option, option, option, option, option, option, option, option, option, option, option, option, option, option, option, option, option, option, option, option, option, option, option, option, option, option, option, option, option, option, option, option, option, option, option, option, option, option, option, option, option, option, option, option, option, option, option, option, option, option, option, option, option, option, option, option, option, option, option, option, option, option, option, option, …]
document.querySelector('select').options.forEach
> ƒ forEach() { [native code] }

This update will allow paste events to retain the heading levels from level h2 to h6.

Up to H6 is supported by govspeak and this makes content more accessible
@DilwoarH DilwoarH force-pushed the bump-paste-govspeak-to-v0.3 branch from 13728cc to cc46a02 Compare July 27, 2022 09:02
@DilwoarH
Copy link
Contributor Author

The audit commit here looks quite eventful. Lots of version changes going on. The failing test seems to be that we lose the ability to do forEach on a HTMLOptionsCollection

$select.options.forEach(function ($el) {

I guess that might be because CoreJS changed version? did it incorrectly polyfill that method? Are our other uses of forEach safe?

This change:

document.querySelector('select').options
> HTMLOptionsCollection(2) [option, option, selectedIndex: 0]
document.querySelector('select').options.forEach
> undefined

Before:

document.querySelector('select').options
> HTMLOptionsCollection(1155) [option, option, option, option, option, option, option, option, option, option, option, option, option, option, option, option, option, option, option, option, option, option, option, option, option, option, option, option, option, option, option, option, option, option, option, option, option, option, option, option, option, option, option, option, option, option, option, option, option, option, option, option, option, option, option, option, option, option, option, option, option, option, option, option, option, option, option, option, option, option, option, option, option, option, option, option, option, option, option, option, option, option, option, option, option, option, option, option, option, option, option, option, option, option, option, option, option, option, option, option, …]
document.querySelector('select').options.forEach
> ƒ forEach() { [native code] }

Hey @kevindew - dropped the lock file upgrades, I think we need to decide on a suitable approach for all of this.

Copy link
Member

@kevindew kevindew left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks @DilwoarH

Do you want to just do a separate PR for this one on the audit? I imagine we've got pretty good test coverage in this repo.

@DilwoarH
Copy link
Contributor Author

Thanks @DilwoarH

Do you want to just do a separate PR for this one on the audit? I imagine we've got pretty good test coverage in this repo.

Yes - probably do the audit updates separately.

@DilwoarH DilwoarH merged commit 0d1747f into main Jul 27, 2022
@DilwoarH DilwoarH deleted the bump-paste-govspeak-to-v0.3 branch July 27, 2022 09:44
@DilwoarH
Copy link
Contributor Author

Thanks @DilwoarH

Do you want to just do a separate PR for this one on the audit? I imagine we've got pretty good test coverage in this repo.

Hey @kevindew - looks like it was a bug and not a feature: zloirock/core-js#988

credits @ollietreend for finding it

DilwoarH pushed a commit that referenced this pull request Jul 27, 2022
`.forEach` was incorrectly made available to all DOM Iterators in core-js. This has now been fixed in the package: zloirock/core-js#988

The recommended approach for HTMLCollections objects is to use `Array.from` (https://developer.mozilla.org/en-US/docs/Web/API/HTMLCollection). This has been applied to the code to support the `.forEach` function.

This was originally noticed when updating the yarn.lock file in PR #2551.

Credits: @ollietreend for finding the solution.
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.

None yet

2 participants