-
Notifications
You must be signed in to change notification settings - Fork 323
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
Deprecate compatibility mode settings #2882
Conversation
ecb4b15
to
94e6bfe
Compare
After a chat with @36degrees and @colinrotherham, we've decided we're going to hold off on this until #2890 is complete. The solution of creating a variable to suppress the warnings locally can be solved by the aforementioned issue and the aforementioned issue can also solve the wider problem of allowing users to suppress warnings. |
94e6bfe
to
2520a13
Compare
#2890 is now complete so this has been updated and is ready to be reviewed again. |
|
||
$govuk-compatibility-govukelements: false !default; | ||
|
||
@if $govuk-compatibility-govukelements == true { | ||
@include _warning("compatibility-mode", "$govuk-compatibility-govukelements " + | ||
"is deprecated. From version 5.0, GOV.UK Frontend will not support compatibility " + |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@owenatgov I think I preferred your wording here? https://github.com/alphagov/govuk-frontend/blob/main/src/govuk/settings/_warnings.scss#L10
Current
…will not support compatibility with the legacy codebase govuk_elements
Suggested
…will remove compatibility with legacy libraries such as govuk_elements
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks Colin! I made a slight tweak to the suggestion so that each warning roughly reads:
{variable} is deprecated. From version 5.0, GOV.UK Frontend will remove compatibility with the legacy library {library}.
I've done this because i'd rather keep each warning separate so that users are clearer on what in their codebase is kicking off the warning and why.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks fab to me, one comment on wording but not a blocker 😊
2520a13
to
3919d13
Compare
This includes adding a private setting: `$_govuk-is-legacy-review-app` which is set to `true` in the legacy app stylesheets within the govuk-frontend local review app. We create this temporary dependancy so that warnings aren't fired locally but are fired for users where compatibility mode settings are true in their own sass.
3919d13
to
eec78d4
Compare
Fixes #2788
What/Why
Deprecates the compatibility mode variables.
Additionally adds a call to
$govuk-suppressed-warnings
in the legacy review app stylesheets to suppress warnings when developing locally.Notes
I've attempted to be consistent in my wording with the warnings introduced in #2844.
This also includes a slight history rewrite in a change to the changelog entry for #2844 to reflect a change to the PR's title to better describe the change and differentiate it from this one.