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 deprecated govuk-main-wrapper and govuk-main-wrapper--l mixins #2385

Merged
merged 2 commits into from
Oct 25, 2021

Conversation

vanitabarrett
Copy link
Contributor

@vanitabarrett vanitabarrett commented Oct 15, 2021

Fixes #1379 (also see here for details of how this is a breaking change for users)

In ade03d4 we deprecated the main-wrapper and main-wrapper--l mixins because we didn't think they were useful.

This PR removes those mixins. Note: the classes remain, but the mixins are being removed.

@govuk-design-system-ci govuk-design-system-ci temporarily deployed to govuk-frontend-pr-2385 October 15, 2021 12:37 Inactive
@vanitabarrett vanitabarrett changed the title Remove previously deprecated main-wrapper and main-wrapper--l mixins Remove deprecated main-wrapper and main-wrapper--l mixins Oct 15, 2021
@govuk-design-system-ci govuk-design-system-ci temporarily deployed to govuk-frontend-pr-2385 October 15, 2021 12:38 Inactive
@vanitabarrett
Copy link
Contributor Author

@EoinShaughnessy I've added a draft entry in the Changelog for this change. Do you want to take a look here or should I copy it into the v4.0.0 draft release notes doc?

@vanitabarrett vanitabarrett changed the title Remove deprecated main-wrapper and main-wrapper--l mixins Remove deprecated govuk-main-wrapper and govuk-main-wrapper--l mixins Oct 15, 2021
@vanitabarrett vanitabarrett force-pushed the remove-main-wrapper-mixins branch from 2d8f953 to 1d13a59 Compare October 15, 2021 12:48
@govuk-design-system-ci govuk-design-system-ci temporarily deployed to govuk-frontend-pr-2385 October 15, 2021 12:48 Inactive
…r--l mixins

In ade03d4
we deprecated the `govuk-main-wrapper` and `govuk-main-wrapper--l` mixins because we didn't think they
were useful.

This commit removes those mixins. Note: the classes remain, but the mixins are being removed.
@vanitabarrett vanitabarrett force-pushed the remove-main-wrapper-mixins branch from 1d13a59 to c77b2cc Compare October 15, 2021 12:50
@govuk-design-system-ci govuk-design-system-ci temporarily deployed to govuk-frontend-pr-2385 October 15, 2021 12:50 Inactive
@vanitabarrett vanitabarrett marked this pull request as ready for review October 15, 2021 13:06
CHANGELOG.md Outdated Show resolved Hide resolved
CHANGELOG.md Outdated Show resolved Hide resolved
@EoinShaughnessy
Copy link
Contributor

@vanitabarrett Content looks good! I can copy it into the draft doc once we've resolved the 2 small comments I've added.

@EoinShaughnessy
Copy link
Contributor

@vanitabarrett Do we want to change the title here, so the release note doesn't read like "This was added in [Remove deprecated...]'? Same kind of rationale as for the change to #1963.

Copy link
Member

@lfdebrux lfdebrux 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

@vanitabarrett vanitabarrett force-pushed the remove-main-wrapper-mixins branch from c77b2cc to 8753d1c Compare October 25, 2021 12:19
@govuk-design-system-ci govuk-design-system-ci temporarily deployed to govuk-frontend-pr-2385 October 25, 2021 12:20 Inactive
@vanitabarrett
Copy link
Contributor Author

@EoinShaughnessy Thanks for those comments - I've made the changes you've suggested.

On the title of the pull request, I'm unsure about renaming to use 'avoid' for work like this. Maybe it's just me, but 'avoid' suggests you shouldn't do a thing but you can if you really need to. The work we're doing is to remove these mixins (if someone were to update to v4.0.0 and try to use them, their code would throw an error). So I'm not sure 'avoid' is a strong enough message? Writing 'avoid deprecated ... etc' also implies the things are still deprecated, but they've actually been removed completely. I'm almost tempted to stick with the title as it is for now and see if anyone picks up on it in research?

@EoinShaughnessy
Copy link
Contributor

@vanitabarrett Yeah, +1 to sticking with the title and seeing what users might say. 👍🏻

@vanitabarrett vanitabarrett merged commit aaef7df into main Oct 25, 2021
@vanitabarrett vanitabarrett deleted the remove-main-wrapper-mixins branch October 25, 2021 12:30
@vanitabarrett vanitabarrett mentioned this pull request Dec 15, 2021
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.

Remove deprecated govuk-main-wrapper mixins
4 participants