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

Remove IE8–10 related Sass and CSS build tasks #3559

Merged
merged 12 commits into from
May 3, 2023

Conversation

querkmachine
Copy link
Member

@querkmachine querkmachine commented Apr 27, 2023

A big one, probably easier to go through it one commit at a time.

Because trimming out Internet Explorer 8–10 support (especially IE8) is such a big chunk of work, I've chosen to only focus on the Sass/CSS elements here. Any JavaScript-related pieces haven't been touched, nor have any aspects related to compatibility mode.

Resolves #3461 and resolves #2622 — except for documentation changes.

Changes

  • Removes instances of the govuk-if-ie8 mixin and any code within them.
  • Removes instances of the govuk-not-ie8 mixin, maintaining any code within them.
  • Removes all Sass settings and mixins relating to IE8.
  • Removes all IE8 related Sass/CSS entry files.
  • Removes tests relating to IE8 CSS generation.
  • Removes references to IE8-specific stylesheets from the review app.
  • Removes IE8-specific build steps from PostCSS configuration.
  • Removes PostCSS dependencies that were only needed for IE8.
  • Removes IE8–10 from Browserslist configuration.
  • Removes IE8 related sections from Sassdoc generation.
  • Removes the block on Autoprefixer updates being made by Dependabot, as IE8 is no longer a blocker to updating.
  • Updates single colon pseudo-element selectors (:before) to the standards compliant double colon (::before). Single colons were only needed for IE8 support.
  • Adds a temporary override to the Stylelint configuration to not raise failures for using double colons. This can be removed when the upstream Stylelint configuration is updated: Remove selector-pseudo-element-colon-notation stylelint-config-gds#36

Notes

This PR doesn't:

  • change anything in Nunjucks/HTML or JavaScript files.
  • touch any CSS relating to compatibility mode, even though the legacy frontend toolkits also ship IE8-compatible code.
  • update any documentation relating to IE8–10.
  • remove any CSS hacks needed for IE11, even if they also apply to older IE versions.

@govuk-design-system-ci govuk-design-system-ci temporarily deployed to govuk-frontend-pr-3559 April 27, 2023 12:38 Inactive
@querkmachine querkmachine self-assigned this Apr 27, 2023
@querkmachine querkmachine requested a review from a team April 27, 2023 12:41
@govuk-design-system-ci govuk-design-system-ci temporarily deployed to govuk-frontend-pr-3559 April 27, 2023 13:00 Inactive
@govuk-design-system-ci govuk-design-system-ci temporarily deployed to govuk-frontend-pr-3559 April 27, 2023 13:05 Inactive
@govuk-design-system-ci govuk-design-system-ci temporarily deployed to govuk-frontend-pr-3559 April 27, 2023 13:16 Inactive
@colinrotherham
Copy link
Contributor

This is brilliant work @querkmachine

I've found a few more bits to remove that fall between the gaps of not-strictly-CSS:

  1. Commit 33382c7 delicately removes app-legacy-ie8.min.css but ?legacy=true mode still loads:

    <link rel="stylesheet" href="/vendor/govuk_template/govuk-template-ie8.css">

  2. There's also the SVG fallback govuk-logotype-crown.png image to delete:

    <img src="{{ params.assetsPath | default('/assets/images') }}/govuk-logotype-crown.png" class="govuk-header__logotype-crown-fallback-image" width="36" height="32" alt="">

    // Hide the inverted crown when printing in browsers that don't support SVG.

  3. Documentation to test in Internet Explorer 8, 9 and 10

    - in [Internet Explorer 8](https://frontend.design-system.service.gov.uk/supporting-ie8/), 9 and 10 - components do not need to look perfect

    Your contribution needs to work with certain browsers as set out in [README](README.md#browser-and-assistive-technology-support). See also [supporting Internet Explorer 8](https://frontend.design-system.service.gov.uk/supporting-ie8/).

    - Internet Explorer 8, 9 and 10, although components may not look perfect

    - [a separate stylesheet](https://frontend.design-system.service.gov.uk/supporting-ie8/) if you support Internet Explorer 8

    This is to ensure compatibility with Internet Explorer 8, which doesn't support the double colon syntax.

    ## Resize text in Internet Explorer versions 8 to 11

@colinrotherham
Copy link
Contributor

colinrotherham commented Apr 28, 2023

Suppose there's always the bits we did incorrectly (ish) on purpose like :before vs ::before

Tagging @kevindew as he's been looking at Stylelint v15 breaking changes recently

@querkmachine
Copy link
Member Author

@colinrotherham Thanks for looking!

For most of them, as you mention, I purposefully left them alone for not being strictly CSS related. The govuk_template reference I imagine being removed when we strip out compatibility mode code.

Good catch on pseudo-elements though, I would include updating those as part of this work!

@kevindew
Copy link
Member

kevindew commented May 2, 2023

Suppose there's always the bits we did incorrectly (ish) on purpose like :before vs ::before

Tagging @kevindew as he's been looking at Stylelint v15 breaking changes recently

Yup, the argument for keeping the single colon was to not actively break compatibility with old IE by using a syntax they don't understand. There was, of course, always going to be a point that other stuff broken in those browers or usage sufficiently insignificant that we could drop it. Maybe we've hit that point? I imagine it's within the scope of your team to decide that.

To make the switch, what I'd suggest doing is opening a draft PR to propose changing the rule in stylelint-config-gds and then changing the rule in this repo's stylelint.config.js with a link to the PR - to basically state, temporary override while we wait for the upstream rules to reflect it.

@govuk-design-system-ci govuk-design-system-ci temporarily deployed to govuk-frontend-pr-3559 May 2, 2023 16:48 Inactive
@govuk-design-system-ci govuk-design-system-ci temporarily deployed to govuk-frontend-pr-3559 May 2, 2023 16:55 Inactive
@querkmachine querkmachine marked this pull request as ready for review May 2, 2023 17:03
@querkmachine querkmachine changed the title Remove IE8-related Sass and CSS build tasks Remove IE8–10 related Sass and CSS build tasks May 3, 2023
@govuk-design-system-ci govuk-design-system-ci temporarily deployed to govuk-frontend-pr-3559 May 3, 2023 07:47 Inactive
* https://github.com/alphagov/stylelint-config-gds/pull/36
* https://stylelint.io/user-guide/rules/list/selector-pseudo-element-colon-notation/
*/
'selector-pseudo-element-colon-notation': 'double'
Copy link
Contributor

Choose a reason for hiding this comment

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

@@ -22,6 +22,14 @@ Instead we recommend checking for the disabled attribute using [`$button.hasAttr

This change was introduced in [pull request #2830: Set the boolean disabled attribute consistently in the button component](https://github.com/alphagov/govuk-frontend/pull/2830).

#### Remove Internet Explorer 8 stylesheets, settings and mixins
Copy link
Contributor

Choose a reason for hiding this comment

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

@querkmachine I liked your suggestion on Slack

Would you mind if we removed this CHANGELOG entry in favour of a (future) combined entry for all the IE8, IE9 and IE10 things we've taken out?

Probably can't say we've stopped supporting IE8 until we've removed the internal documentation too

@colinrotherham colinrotherham self-requested a review May 3, 2023 10:50
@govuk-design-system-ci govuk-design-system-ci temporarily deployed to govuk-frontend-pr-3559 May 3, 2023 12:20 Inactive
@colinrotherham colinrotherham changed the base branch from main to review-app-user May 3, 2023 12:20
@govuk-design-system-ci govuk-design-system-ci temporarily deployed to govuk-frontend-pr-3559 May 3, 2023 12:22 Inactive
Copy link
Contributor

@colinrotherham colinrotherham left a comment

Choose a reason for hiding this comment

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

✅ All looks good, approving

I've rebased this into the new src packages/govuk-frontend/src directory layout so hold off merging for now until we get main as the new base (once #3491 merges)

Removes uses of the `govuk-if-ie8` and `govuk-not-ie8` mixins from Sass, removing IE8 specific code in the process, whilst maintaing any non-IE8 code.

This commit does not remove the mixins themselves.
Clean up some other code which wasn't specifically scoped to IE8, but where the code structure had been altered to accommodate IE8.
@govuk-design-system-ci govuk-design-system-ci temporarily deployed to govuk-frontend-pr-3559 May 3, 2023 12:41 Inactive
@querkmachine querkmachine merged commit 8586ba4 into review-app-user May 3, 2023
@querkmachine querkmachine deleted the remove-ie8-css branch May 3, 2023 13:04
@colinrotherham
Copy link
Contributor

Ah you've accidentally merged into the other PR @querkmachine

Did you want to revert until we're ready for main?

I don't mind, but it might mean this PR needs opening again

@querkmachine querkmachine restored the remove-ie8-css branch May 3, 2023 13:24
@querkmachine
Copy link
Member Author

@colinrotherham Ah crap, sorry, the hold on merging totally slipped my mind.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
4 participants