Skip to content
This repository has been archived by the owner on Mar 13, 2024. It is now read-only.

MM-33003 Remove a bunch of unneeded mixins used in the sass code #9364

Merged
merged 19 commits into from
Dec 7, 2021

Conversation

jespino
Copy link
Member

@jespino jespino commented Nov 12, 2021

Summary

A lot of mixins are used for backward CSS3 compatibility, but most of the
browsers already supports most of the CSS3 features, so we are no longer
needing that mixins. The resulting css code is longer without giving us
anything. Also I guess the compile takes a bit more because the need os pass
though the compass mixins.

Also, compass-mixins is unmaintained (not a big deal) so we can get rid of that
dependency if we remove all the mixins used from there.

UPDATE: I already removed all the mixins so I removed the compass-mixins dependency.

Some interesting numbers.

Before the changes, the css generate is 35020 lines (740K aprox) and is taking 1.6 seconds of CPU to execute.
After the changes, the css generated is 33884 lines (700K aprox) and is taking 1.4-1.5 seconds of CPU to execute.

Ticket Link

https://mattermost.atlassian.net/browse/MM-33003

Release Note

NONE

@jespino jespino requested review from hmhealey and deanwhillier and removed request for hmhealey November 12, 2021 11:14
@jespino jespino added the 2: Dev Review Requires review by a core commiter label Nov 12, 2021
@hmhealey hmhealey changed the title Remove a bunch of unneeded mixins used in the sass code MM-33003 Remove a bunch of unneeded mixins used in the sass code Nov 15, 2021
@hmhealey
Copy link
Member

I attached this to the ticket I had for upgrading or removing compass-mixins. Feel free to make a new ticket instead if you had one you wanted to make

Copy link
Member

@hmhealey hmhealey left a comment

Choose a reason for hiding this comment

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

Looks good to me! I'm sure most of that stuff is from back before we dropped support for IE10 at least

Copy link
Contributor

@deanwhillier deanwhillier left a comment

Choose a reason for hiding this comment

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

LGTM. Some non-blocking comments for now or later :)

sass/layout/_markdown.scss Outdated Show resolved Hide resolved
sass/routes/_settings.scss Outdated Show resolved Hide resolved
sass/routes/_settings.scss Outdated Show resolved Hide resolved
sass/utils/_mixins.scss Show resolved Hide resolved
sass/base/_structure.scss Show resolved Hide resolved
@jespino jespino added 3: QA Review Requires review by a QA tester and removed 2: Dev Review Requires review by a core commiter labels Nov 17, 2021
@jespino
Copy link
Member Author

jespino commented Nov 22, 2021

/update-branch

@jespino jespino requested review from jgilliam17 and removed request for furqanmlk November 29, 2021 09:27
@jgilliam17
Copy link
Contributor

/update-branch

@jgilliam17 jgilliam17 added the Setup Cloud Test Server Setup a test server using Mattermost Cloud label Dec 6, 2021
@jgilliam17
Copy link
Contributor

Thanks @jespino
Tested, looks good to merge.

@jgilliam17 jgilliam17 added 4: Reviews Complete All reviewers have approved the pull request and removed 3: QA Review Requires review by a QA tester Setup Cloud Test Server Setup a test server using Mattermost Cloud labels Dec 7, 2021
@mm-cloud-bot
Copy link

Test server destroyed

@jespino jespino merged commit 5c07315 into mattermost:master Dec 7, 2021
@amyblais amyblais added Changelog/Not Needed Does not require a changelog entry Docs/Not Needed Does not require documentation labels Dec 7, 2021
@amyblais amyblais removed Changelog/Not Needed Does not require a changelog entry Docs/Not Needed Does not require documentation labels Dec 15, 2021
@amyblais
Copy link
Member

/cherry-pick release-6.3

@mattermod
Copy link
Contributor

Cherry pick is scheduled.

mattermost-build pushed a commit to mattermost-build/mattermost-webapp that referenced this pull request Dec 21, 2021
…termost#9364)

* Removing no longer needed backward compatibility mixins for CSS3

* Removing usage of @border-radius mixin

* Removing more unneeded mixins

* Removing more unneeded mixins

* Removing more unneeded mixins

* Removing more unneeded mixins

* Fix linter errors

* Removing single-transition mixin usage

* Fix linter errors

* Removing input-placeholder and background-size mixins

* Removing more mixins usage

* Removing entirely the compass-mixins dependency

* Removing compass-mixins imports

* applying review suggestions

* fix linter problems

Co-authored-by: Mattermod <[email protected]>
(cherry picked from commit 5c07315)
@mattermod mattermod added the CherryPick/Done Successfully cherry-picked to the quality or patch release tracked in the milestone label Dec 21, 2021
@amyblais amyblais added this to the v6.3.0 milestone Dec 21, 2021
@amyblais amyblais added Changelog/Not Needed Does not require a changelog entry Docs/Not Needed Does not require documentation labels Dec 21, 2021
cleferman pushed a commit to cleferman/mattermost-webapp that referenced this pull request Dec 21, 2021
…termost#9364)

* Removing no longer needed backward compatibility mixins for CSS3

* Removing usage of @border-radius mixin

* Removing more unneeded mixins

* Removing more unneeded mixins

* Removing more unneeded mixins

* Removing more unneeded mixins

* Fix linter errors

* Removing single-transition mixin usage

* Fix linter errors

* Removing input-placeholder and background-size mixins

* Removing more mixins usage

* Removing entirely the compass-mixins dependency

* Removing compass-mixins imports

* applying review suggestions

* fix linter problems

Co-authored-by: Mattermod <[email protected]>
mattermod pushed a commit that referenced this pull request Dec 22, 2021
cleferman pushed a commit to cleferman/mattermost-webapp that referenced this pull request Dec 22, 2021
…termost#9364)

* Removing no longer needed backward compatibility mixins for CSS3

* Removing usage of @border-radius mixin

* Removing more unneeded mixins

* Removing more unneeded mixins

* Removing more unneeded mixins

* Removing more unneeded mixins

* Fix linter errors

* Removing single-transition mixin usage

* Fix linter errors

* Removing input-placeholder and background-size mixins

* Removing more mixins usage

* Removing entirely the compass-mixins dependency

* Removing compass-mixins imports

* applying review suggestions

* fix linter problems

Co-authored-by: Mattermod <[email protected]>
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
4: Reviews Complete All reviewers have approved the pull request Changelog/Not Needed Does not require a changelog entry CherryPick/Done Successfully cherry-picked to the quality or patch release tracked in the milestone Docs/Not Needed Does not require documentation release-note-none
Projects
None yet
7 participants