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 list-style for govspeak ordered lists #3413

Merged
merged 2 commits into from
Jun 2, 2023
Merged

Conversation

kevindew
Copy link
Member

This removes setting list-style on ordered lists to instead allow the
use of the type attribute on an ordered list to specify the list-style.

I couldn't determine why this style was needed. Current browsers style
this as list-style decimal by default and I can't find any code this
undoes. It could be that previously this was to undo an earlier
override, but it would be very unconventional for current GOV.UK to
style an ol without a class so this seems unlikely.

The motivation for making this change is to support some niche behaviour
used by publishers that will become disabled. Currently some advanced
published have managed to published ordered lists that are alphabetical
or roman numeral prefixed by use of an ol element with a style
attribute. This will not be available with the role out of the GOV.UK
Content Security Policy which forbids unsafe inline styles 1.

Visual Changes

Before

Screenshot 2023-05-23 at 17 50 59

After

Screenshot 2023-05-23 at 17 50 38

@govuk-ci govuk-ci temporarily deployed to components-gem-pr-3413 May 23, 2023 16:51 Inactive
@kevindew kevindew force-pushed the govspeak-ordered-lists branch from f1071e0 to 6db3373 Compare May 23, 2023 16:52
@govuk-ci govuk-ci temporarily deployed to components-gem-pr-3413 May 23, 2023 16:52 Inactive
@MartinJJones
Copy link
Contributor

The changes look good to me.

I was reading further into the use of the type attribute for ordered lists out of interest on MDN - https://developer.mozilla.org/en-US/docs/Web/HTML/Element/ol#type, which advises:

Note: Unless the type of the list number matters (like legal or technical documents where items are referenced by their number/letter), use the CSS list-style-type property instead.

and also in the W3C spec - https://www.w3.org/TR/2012/WD-html-markup-20120329/ol.html

Note: The type attribute on the ol element was deprecated in a previous version of HTML, but is no longer deprecated, as it has meaning and is not simply presentational.

I imagine the type does matter for the documents publishers have used this inline style approach on previously. Either way, looks like an improvement to me 👍

Copy link
Contributor

@MartinJJones MartinJJones left a comment

Choose a reason for hiding this comment

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

Changes look good to me 👍

kevindew added 2 commits June 2, 2023 09:16
This list-style-position is defined for ul in the CSS above this and
thus is redundant.
This removes setting list-style on ordered lists to instead allow the
use of the type attribute on an ordered list to specify the list-style.

I couldn't determine why this style was needed. Current browsers style
this as list-style decimal by default and I can't find any code this
undoes. It could be that previously this was to undo an earlier
override, but it would be very unconventional for current GOV.UK to
style an `ol` without a class so this seems unlikely.

The motivation for making this change is to support some niche behaviour
used by publishers that will become disabled. Currently some advanced
published have managed to published ordered lists that are alphabetical
or roman numeral prefixed by use of an `ol` element with a style
attribute. This will not be available with the role out of the GOV.UK
Content Security Policy which forbids unsafe inline styles [1].

[1]: alphagov/govuk_app_config#291
@kevindew kevindew force-pushed the govspeak-ordered-lists branch from 6db3373 to 132aa56 Compare June 2, 2023 08:17
@govuk-ci govuk-ci temporarily deployed to components-gem-pr-3413 June 2, 2023 08:17 Inactive
@kevindew
Copy link
Member Author

kevindew commented Jun 2, 2023

Thanks @MartinJJones, yup I read the same spec and figured this was appropriate usage of the attribute as it used to add semantics to the copy.

@kevindew kevindew merged commit ad49ca6 into main Jun 2, 2023
@kevindew kevindew deleted the govspeak-ordered-lists branch June 2, 2023 08:24
@JamesCGDS JamesCGDS mentioned this pull request Jun 8, 2023
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