Merge upstream #125
Open
Merge upstream #125
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Group helpers and objects into logical groups
Add collapseOnMobile breadcrumbs flag
Co-authored-by: Oliver Byford <[email protected]>
…on-item-html Add the ability to specify HTML for a navigation item
…dist_docs Update README with new link to installing using compiled files
Update pending 3.7.0 (unreleased) changelog with style edits
Remove duplication of 'pull request' in changelog
…readcrumbs Deep link to guidance on collapsing breadcrumbs
Help users understand whether this change affects them by tying it to the deprecation warning – if they see the warning, they need to make a change; if they don't, no change is required.
Release v3.7.0 🚀
When serviceName is used in the header, it should be in conjunction with serviceUrl, otherwise it renders a link with an empty href.
Add serviceUrl to latest header example which includes serviceName
As raised in alphagov#1780, lowercasing this attribute causes issue with Angular. I've searched the codebase for any other camel-cased SVG attributes and viewBox is the only one that we use: ``` $ ag 'allowReorder|attributeName|attributeType|autoReverse|baseFrequency|baseProfile|calcMode|clipPathUnits|contentScriptType|contentStyleType|diffuseConstant|edgeMode|externalResourcesRequired|filterRes|filterUnits|glyphRef|gradientTransform|gradientUnits|kernelMatrix|kernelUnitLength|keyPoints|keySplines|keyTimes|lengthAdjust|limitingConeAngle|markerHeight|markerUnits|markerWidth|maskContentUnits|maskUnits|numOctaves|pathLength|patternContentUnits|patternTransform|patternUnits|pointsAtX|pointsAtY|pointsAtZ|preserveAlpha|preserveAspectRatio|primitiveUnits|referrerPolicy|refX|refY|repeatCount|repeatDur|requiredExtensions|requiredFeatures|specularConstant|specularExponent|spreadMethod|startOffset|stdDeviation|stitchTiles|surfaceScale|systemLanguage|tableValues|targetX|targetY|textLength|viewBox|viewTarget|xChannelSelector|yChannelSelector|zoomAndPan' src src/govuk/components/footer/template.njk 61: viewBox="0 0 483.2 195.7" src/govuk/components/button/template.njk 27:<svg class="govuk-button__start-icon" xmlns="http://www.w3.org/2000/svg" width="17.5" height="19" viewBox="0 0 33 40" aria-hidden="true" focusable="false"> src/govuk/components/header/template.njk 24: viewBox="0 0 132 97" src/govuk/assets/images/govuk-mask-icon.svg 1:<svg xmlns="http://www.w3.org/2000/svg" width="132" height="97" viewBox="0 0 132 97" version="1.1"> ``` Fixes alphagov#1780.
Fix typo in release-to-branch script
Correctly camel case SVG attributes in the header and footer
When the user navigates back to a previous page that includes a conditional reveal, the visible state of the conditionally revealed content is currently only preserved in some browsers.
Firefox / Safari
----------------
Recent versions of Firefox and Safari use a Back-Forward Cache ('bfcache')[1] which preserves the entire page, which means the JavaScript is not re-evaluated but the browser 'remembers' that the conditional reveal was visible.
Internet Explorer
-----------------
In Internet Explorer the state of the form controls has not been restored at the point we currently call the init function, and so the conditional reveal state is not preserved.
To fix this, wait for the `DOMContentLoaded` event before syncing the visibility of the conditional reveals to their checkbox / radio state. As the checkbox state in IE8-11 has been restored before the `DOMContentLoaded` event fires, this fixes the preservation of the reveal state in Internet Explorer.
We already polyfill document.addEventListener and the DOMContentLoaded event for IE8, so we don't have to treat it as a special case.
Edge Legacy
-----------
In Edge 18, the state of the form controls does not seem to be restored at all when navigating back, so there is nothing to sync the conditional reveal state to 😢
Chromium based browsers (Chrome, Edge, Opera)
---------------------------------------------
In browsers based on Chromium 78 or older, the state of the form controls has been restored at the point we invoke the init function, and so the reveal state currently displays correctly, even without waiting for the `DOMContentLoaded` event.
In Chromium 79, the 'timing of restoring control state was changed so that it is done as a task posted just after DOMContentLoaded' [2]. This means that even after the `DOMContentLoaded` event the form state has not yet been restored. The recommended approach seems to be to wait for the `pageshow` event, however in Chromium 79 the form control state has not been restored at the point this event fires either [3]! This was fixed in Chromium 80 [4].
So:
- in Chrome ≤ 78, the form control state is restored before the script is evaluated, before both the `DOMContentLoaded` and `pageshow` events.
- in Chrome = 79, the form control state is restored after both the `DOMContentLoaded` and `pageshow` events.
- in Chrome ≥ 80, the form control state is restored after the `DOMContentLoaded` but before the `pageshow` event.
Syncing the conditional reveal state after the `pageshow` event preserves the conditional reveal state except for Chromium 79 where it remains broken. Given that Chrome 79's usage is already trending towards 0 [5] (0.29% in May 2020) and there's seemingly no other event we can listen for that'll guarantee the state is restored, we'll accept that it'll remain broken in this specific version (affecting Chrome 79, Edge 79 and Opera 66).
The `pageshow` event is not supported in older browsers including IE8-10 so we need to listen for both. This means that we 'sync' twice in browsers that support `pageshow`, but the performance impact should be minimal.
[1]: https://www.chromestatus.com/feature/5815270035685376
[2]: https://chromium.googlesource.com/chromium/src.git/+/069ad65654655b7bdfb9b760f188395840bc4be4
[3]: https://bugs.chromium.org/p/chromium/issues/detail?id=1035662
[4]: https://crrev.com/069ad65654655b7bdfb9b760f188395840bc4be4
[5]: https://gs.statcounter.com/browser-version-market-share
Because of a bug in IE9-10 the readyState as being reported as interactive when we're checking it, which means that the DOMContentLoaded event handler isn't set up, and we try to sync the conditional reveals before the form control state has been restored. Instead: - sync on the pageshow event OR the DOMContentLoaded if the browser does not support the pageshow event - always sync within the init function (to handle cases where init is called after both the pageshow and DOMContentLoaded events have already fired)
These seem to be just developer mistakes.
This rule is defined in stylelint-config-standard [1] as:
```
"rule-empty-line-before": [
"always-multi-line",
{
except: ["first-nested"],
ignore: ["after-comment"],
},
],
```
These violations are fixes for where there is a new line provided for
the first nested rule.
[1]: https://github.com/stylelint/stylelint-config-standard/blob/00d8b36bab00fb20426ca226338fccc2ee34f1f0/index.js#L82-L88
The only file with a violation for this is font-face where the public CSS comment is intentional.
There was already a rule for this in scss:lint however the implementation there didn't seem to be sensitive enough to catch these violations.
This rule prevents using selectors that target elements not defined in HTML, SVG or MathML specs [1]. The only violation for this is an old fix for Firefox.
The stylelint-config-standard specifies lower case for values [1] [1]: https://github.com/stylelint/stylelint-config-standard/blob/00d8b36bab00fb20426ca226338fccc2ee34f1f0/index.js#L102
stylelint-config-standard allows 0 empty lines in a value list [1]. Since these violations appear to be added to break the content up into logical groupings I've added a disable rather than remove the new lines. [1]: https://github.com/stylelint/stylelint-config-standard/blob/00d8b36bab00fb20426ca226338fccc2ee34f1f0/index.js#L108
I've disabled the stylelint rule in device-pixels and spacing files as stylelint and sass-lint conflict over what they believe the indentation should be.
This enforces that psuedo elements are defined with a single colon. In the config/.sass-lint.yml file the rule for pseudo-elements was disabled which meant this project has a mix of both single colon and double colon pseudo elements. The rule to use single colon rather than double was requested by GOV.UK frontend developers as a means to not purposely break IE8 despite a lack of a official support, see alphagov/stylelint-config-gds#1 for more information.
This ports the rules from config/.sass-lint.yml [1] for ordering CSS properties. This ordering rule seems to be distinct to govuk-frontend and not adopted in other parts of GDS (as far as I'm aware) so I've set this rule as an extension specifically for govuk-frontend. There is probably a conversation to happen at some point as to whether this should be adopted wider or whether govuk-frontend wish to continue with it. [1]: https://github.com/alphagov/govuk-frontend/blob/2f73bd6d639db10c61d3b0da6f79f08150a4e530/config/.sass-lint.yml#L250-L405
This rule was accidentally missed in the initial config for stylelint-config-gds. This applies disabling of this rule in the situations where we make an exception and want important rules.
In stylelint 13.7.0 they now treat multiline SCSS comments as a single comment [1] which means the disabling techniques we previously used no longer work. By adopting this new syntax the reasons are also understood to be associated with the rule and can allow switching on the reportDescriptionlessDisables option [2] which can require the disabling of rules to always be documented. For example ``` // comment describing disabling // stylelint-disable indentation ``` is treated as a comment of: "comment describing disabling stylelint-disable indentation" Instead this can be resolved with the following technique: ``` // stylelint-disable indentation -- comment describing // disabling ``` This therefore updates all of the stylelint disabling of rules to match this pattern. [1]: stylelint/stylelint#4886
Switch linting to stylelint
Looks like this doesn't exist in GOVUK Frontend at the moment, so removing this even though it's not directly related to this change.
Remove sass lint
…g-doc Add missing step to publishing docs
We ended up with two $_govuk-colour-palette-legacy as a result of a messy rebase when switching from sass-lint to stylelint. This removes the duplicate definition.
…y-colour-palette Remove duplicate $_govuk-colour-palette-legacy
Closed
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
PR na merge s upstream - vysoka pravdepodobnost, ze niektore veci prestanu fungovat