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

[4.0-dev] Fixing SCSS issues with division deprecation and math.div() #35648

Closed
wants to merge 26 commits into from

Conversation

thednp
Copy link
Contributor

@thednp thednp commented Sep 22, 2021

Pull Request for PR #35639 .

Important

This PR could be merged into [4.1-dev] as well.

Summary of Changes

  • The fontawesome icon library haven't marked their $fa-fw-width variable with !default, see #17482 which uses (20em / 16) expression which is deprecated in dart-sass 2.0 and also largely reported in the fontawesome issues.

  • Other changes in this PR also address other dart-sass issues, but we don't replace for instance 24 / 16 with math.div(24, 16) as recommended by the compiler, but with 24 * 0.0625 (which is the result of 1/16) and the reason for that is the libsass compiler still largely used and this approach is also widely adopted. I suspect scssPhp also might not have math implemented.

  • See this PR in fontawesome for more info. Briefly, Bootstrap core devs also continue to support libsass, so should we.

Testing Instructions

Check SASS compiler output. You should see all SCSS compiled. If the npm build is green, there is no need to test anything else.

Actual result BEFORE applying this Pull Request

The SCSS compiler just won't do division via the / (forward slash) operator, the build will simply fail
https://ci.joomla.org/joomla/joomla-cms/47599

Expected result AFTER applying this Pull Request

All SCSS files compiled successful.

Documentation Changes Required

No.

thednp and others added 16 commits September 22, 2021 20:37
Co-authored-by: Tobias Zulauf <zero-24@users.noreply.github.com>
* Add a new Icon for com_tags to the quicicon module

* Update administrator/language/en-GB/com_tags.ini

Co-authored-by: Brian Teeman <brian@teeman.net>

Co-authored-by: Brian Teeman <brian@teeman.net>
…omla#31675)

* Actionlogs: Added parameter to disable relative timestamps.

* Update administrator/components/com_actionlogs/config.xml

Co-authored-by: Quy <quy@fluxbb.org>

* Apply suggestions from code review

Co-authored-by: Quy <quy@fluxbb.org>

Co-authored-by: Quy <quy@fluxbb.org>
* log:logDeprecated

* use new log method

* new log method, improve callStack lookup

* phpcs

* phpcs

* Check deprecation logging on boot

* trigger_error everywhere

* use namespaced classes
* add Joomla! Accessibility Team to the code owners

* add cassiopeia

* add everything in the templates
@joomla-cms-bot joomla-cms-bot added NPM Resource Changed This Pull Request can't be tested by Patchtester PR-4.0-dev labels Sep 22, 2021
@thednp
Copy link
Contributor Author

thednp commented Sep 22, 2021

@brianteeman I've completed the PR for [4.0-dev] it should be a valid build. After this merge, the same changes are required for [4.1-dev] asap. I've updated the description. Please let me know if this PR needs anything.

@brianteeman
Copy link
Contributor

I tried to test this but in order to do so I need to be able to replicate the problem that this is fixing.

I have been unable to replicate the problem

@thednp
Copy link
Contributor Author

thednp commented Sep 25, 2021

@brianteeman it may be impossible to replicate the issue right now. The stylelint-scss basically doesn't pick on this issue, because it's more of a warning for deprecated SCSS and the build might fail once in a while, depending on when and how SASS compiler (dart-sass based) "feels" the warning should kick in.

I can try to write a step-by-step guide on how you might replicate the problem, but once "sass": "^1.32.12", goes to "sass": "^2.0.0", this will be 100% a problem for anyone trying to contribute to Joomla.

  • try and fork the [4.0-dev] branch into [brian-test-scss] branch
  • edit any SCSS file and try changing for instance 16px -> 17px, commit and see if NPM build is successful
  • edit 16px -> 16pe, using a wrong unit for the value, commit and see if NPM build is successful
  • rename a SCSS file so that the compiler cannot find it with its autoresolve capability, commit and see if NPM build is successful

I think that for a minor SCSS consistency can pass through the stylelint-scss but can trigger an entire cascade of warnings that simply stop the build process.

@thednp
Copy link
Contributor Author

thednp commented Oct 4, 2021

@brianteeman @dgrammatiko what's up with this appveyor issue?

@wilsonge
Copy link
Contributor

I'm tempted to sit on the FA Fix until they fix upstream (it is 1.5 months out which is a bit annoying per FortAwesome/Font-Awesome#18371 ) - but saves us overriding warnings for the sake of it. Obviously the rest of the fixes are good.

@thednp
Copy link
Contributor Author

thednp commented Oct 24, 2021

Well thanks for someone finally say anything. Cheers

@ghazal
Copy link
Contributor

ghazal commented Nov 14, 2021

The math.div issue can be resolved locally by applying (and fiddling with) the recommendations in the article mentioned earlier (https://sass-lang.com/documentation/breaking-changes/slash-div), even the fontawesome-free issue.
Obviously though, one has to wait for the official update.


This comment was created with the J!Tracker Application at issues.joomla.org/tracker/joomla-cms/35648.

@dgrammatiko
Copy link
Contributor

@wilsonge maybe you should press the green button here?

@chmst chmst changed the base branch from 4.0-dev to 4.1-dev January 31, 2022 16:16
@chmst
Copy link
Contributor

chmst commented Jan 31, 2022

@thednp could you please have a look here and resolve conflicts?

@thednp
Copy link
Contributor Author

thednp commented Jan 31, 2022

@chmst unfortunately this is too late for me to do anything, I don't have write access here anymore.

image

@brianteeman
Copy link
Contributor

You never did - that message is related to who can merge the file into core.

@thednp
Copy link
Contributor Author

thednp commented Jan 31, 2022

Not true, if changes after my PR would have been into less than 3 files, I could resolve conflicts. I used to do that for the package-lock.json for the color-picker PR. Both resolve cases are for you to resolve.

@dgrammatiko
Copy link
Contributor

@thednp @brianteeman @chmst I could rebase this PR for 4.1 but are you willing to test it so it could be finally merged?

@brianteeman
Copy link
Contributor

@dgrammatiko I took a look at doing it but the 8 files here are only a small number of the ones that need updating

@dgrammatiko
Copy link
Contributor

@brianteeman you mean there are still deprecation notices after applying these changes?

@brianteeman
Copy link
Contributor

Deleted my post as its the same thing I posted earlier and I think it might have been my fault

@brianteeman
Copy link
Contributor

brianteeman commented Jan 31, 2022

actually no idea. The error report AFTER my version of this pr is the same as #35648 (comment) from september

@thednp
Copy link
Contributor Author

thednp commented Jan 31, 2022

@dgrammatiko IDK man, I'm out of touch with this lately. I would say go ahead and if you get more deprecation notice, have a look at my changes and apply same methods.

@dgrammatiko
Copy link
Contributor

#36906 should fix this but:

  • updates font awesomeme to 6.0.0-beta3
  • localises the choices 2 css files (till the changes are done upstream)

@HLeithner HLeithner changed the base branch from 4.1-dev to 4.2-dev June 27, 2022 13:07
@HLeithner
Copy link
Member

This pull request has automatically rebased to 4.2-dev.

@dgrammatiko
Copy link
Contributor

This was already fixed with another pr

@Quy
Copy link
Contributor

Quy commented Jun 27, 2022

Fixed per #37255

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Conflicting Files NPM Resource Changed This Pull Request can't be tested by Patchtester
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet