THREESCALE-9588: Migrate from sass-rails to dartsass-rails #4145
Conversation
mayorova
force-pushed
the
remove-sass-rails
branch
from
efee115 to
82e1d65
Compare
Gemfile
Outdated
|
|
||
| group :assets do | ||
| gem 'font-awesome-rails', '~> 4.7.0.5' | ||
| gem 'font-awesome-sass', '~> 5.15.1' |
There was a problem hiding this comment.
According to the comment in _authentication_providers.scss, we could not upgrade to FontAwesome v5, because of using compass, which is now not an issue:
// HACK: Patternfly 4 uses Font Awesome 5. Icons are now separated in sets (regular, solid, brand)
// and github belongs to brand. As a workaround, force its font-family so that it comes from FA 4.
// font-awesome-rails (FA 4) can't be updated to font-awesome-sass, it is not compatible with
// our sass-rails.
// TODO: Upgrade sass-rails, which we couldn't do before we got rid of compass
Gemfile.lock
Outdated
| font-awesome-rails (4.7.0.8) | ||
| railties (>= 3.2, < 8.0) | ||
| font-awesome-sass (5.15.1) | ||
| sassc (>= 1.11) |
There was a problem hiding this comment.
Oh maybe this is the reason why we still need sassc...
It's probably would be interesting to try and use font-awesome that it baked into PF4 (in node_modules), so we can get rid of this dependency?
Not sure it's feasible though, maybe @josemigallas would know...
| @@ -0,0 +1,14 @@ | |||
| //= link_tree ../images | |||
There was a problem hiding this comment.
I'm not quite sure this file is 100% correct... Will need further testing.
For reference: https://github.com/rails/sprockets/blob/main/UPGRADING.md#manifestjs
There was a problem hiding this comment.
Well, after working on this for a while, having multiple iterations and testing, it seems that this is correct :D
| @@ -1,5 +1,6 @@ | |||
| @import "provider/typography"; | |||
| @import "provider/logo"; | |||
| @use 'provider/colors' as *; | |||
There was a problem hiding this comment.
Unlike @import, @use namespaces the imported stuff like functions, mixins and variables, so you would need to access them as such: typography.line-height-times(24), colors.$border-color etc.
Adding as * expoxes all of these in the current scope, so you can avoid adding the prefix.
Reference: https://sass-lang.com/documentation/at-rules/use/
| @@ -1,18 +0,0 @@ | |||
| .fa-auth0:before { | |||
There was a problem hiding this comment.
So, I decided to remove this. @extend .fa-star was not working (because of font-awesome dependency change), and moreover, for a long time I thought that having this was quite stupid.
Like - why is it even a good idea to show a generic star icon as a "logo" of Auth0??
Or a ❮❯ text as a "logo" of RH-SSO? (like - yeah, because it's "similar" to Keycloak's logo...).
By the way, GitHub's logo is already not shown in production, not sure at what point it got broken, but well, I think GitHub doesn't need a logo either, the name is enough.
| $secondary-title-font-size: 1.25rem; // --pf-c-content--h2--FontSize | ||
| $sub-title-font-size: (5/4) * $fontSize; | ||
| $caption-font-size: (7/8) * $fontSize; | ||
| $sub-title-font-size: math.div(5,4) * $fontSize; |
There was a problem hiding this comment.
Here and everywhere, these changes are fixing the following warning:
Deprecation Warning [slash-div]: Using / for division outside of calc() is deprecated and will be removed in Dart Sass 2.0.0.
Recommendation: math.div(5, 4) or calc(5 / 4)
More info and automated migrator: https://sass-lang.com/d/slash-div
╷
42 │ $sub-title-font-size: (5/4) * $fontSize;
│ ^^^
╵
app/assets/stylesheets/provider/_typography.scss 42:31 @use
app/assets/stylesheets/error.scss 1:1 root stylesheet
| @@ -1,11 +1,15 @@ | |||
| @use "provider/colors" as *; | |||
| @use "provider/typography" as *; | |||
| @use 'sass:math'; | |||
There was a problem hiding this comment.
Needed for math.div
| font-family: FontAwesome; | ||
| font-weight: $font-weight-normal; | ||
| @mixin iconic($content, $font-weight: $font-weight-normal) { | ||
| font-family: "Font Awesome 5 Free"; |
There was a problem hiding this comment.
After removing font-awesome-rails we're now using version 5 of font-awesome, which requires a font-family update.
| font-family: "Font Awesome 5 Free"; | ||
| font-weight: $font-weight; | ||
| font-style: normal; | ||
| display: inline-block; |
There was a problem hiding this comment.
Removing this just because it was duplicated.
| &.delete:before { @include iconic("\f014\0000a0"); } // trash | ||
| &.refresh:before { @include iconic("\f021\0000a0"); } // resend | ||
| &.edit:before { @include iconic("\f040\0000a0"); } // pencil | ||
| &.delete:before { @include iconic("\f2ed\0000a0"); } // trash |
There was a problem hiding this comment.
I used fas fa-trash-alt because it looked similar to the previous version. See:
before
after
However I noticed later that some new PF4 screens use fas fa-trash, so maybe need to change this code to f1f8.
| &.undo:before, &.revert:before { @include iconic("\f0e2\0000a0"); } | ||
| // For reference: | ||
| // https://github.com/FortAwesome/font-awesome-sass/blob/5.15.1/assets/stylesheets/font-awesome/_variables.scss#L1319 | ||
| &.undo:before, &.revert:before { @include iconic("\f2ea\0000a0"); } |
There was a problem hiding this comment.
Original: https://fontawesome.com/v4/icon/undo
New: https://fontawesome.com/v5/icons/undo-alt?f=classic&s=solid
Optinally, we can update to a slightly different https://fontawesome.com/v5/icons/undo?f=classic&s=solid (in case this is what PF4 uses)
| &.refresh:before { @include iconic("\f021\0000a0"); } // resend | ||
| &.edit:before { @include iconic("\f040\0000a0"); } // pencil | ||
| &.delete:before { @include iconic("\f2ed\0000a0"); } // trash | ||
| &.refresh:before { @include iconic("\f2f1\0000a0"); } // resend |
There was a problem hiding this comment.
Original: https://fontawesome.com/v4/icon/refresh
New: https://fontawesome.com/v5/icons/sync-alt?f=classic&s=solid
We could alternatively use https://fontawesome.com/v5/icons/sync?f=classic&s=solid, similar to undo icon.
| &.edit:before { @include iconic("\f040\0000a0"); } // pencil | ||
| &.delete:before { @include iconic("\f2ed\0000a0"); } // trash | ||
| &.refresh:before { @include iconic("\f2f1\0000a0"); } // resend | ||
| &.edit:before { @include iconic("\f303\0000a0"); } // pencil |
There was a problem hiding this comment.
| &.off:before, &.disable:before, &.suspend:before { @include iconic("\f28b\0000a0"); } // off | ||
| &.activate:before, &.enable:before, &.resume:before { @include iconic("\f144\0000a0"); } // unlock | ||
| &.message:before { @include iconic("\f003\0000a0"); } // unlock | ||
| &.message:before { @include iconic("\f0e0\0000a0"); } // unlock <i class="far fa-envelope"></i> |
There was a problem hiding this comment.
Original: https://fontawesome.com/v4/icon/envelope-o
In v5 it's different in fas (standard) style: https://fontawesome.com/v5/icons/envelope?f=classic&s=solid
In far style it's the same as the previous one: https://fontawesome.com/v5/icons/envelope?f=classic&s=regular
But I don't know how to force this style with just unicode code. Font-weight didn't work.
I think it looks OK though:
before:
after:
| @@ -1,72 +1,70 @@ | |||
| @import 'provider/typography'; | |||
| @forward 'provider/typography'; | |||
There was a problem hiding this comment.
I guess @forward is appropriate in this file:
https://sass-lang.com/documentation/at-rules/forward/
| $integration-tab-active-border: $border-width solid $border-color; | ||
| $integration-tab-warning-bg-color: $dirty-color; | ||
| $integration-tab-warning-color: darken($dirty-color, 60%); | ||
| $integration-tab-warning-color: color.adjust($dirty-color, $lightness: -60%); |
There was a problem hiding this comment.
Fixing
Deprecation Warning [global-builtin]: Global built-in functions are deprecated and will be removed in Dart Sass 3.0.0.
Use color.adjust instead.
More info and automated migrator: https://sass-lang.com/d/import
╷
9 │ $integration-tab-warning-color: darken($dirty-color, 60%);
│ ^^^^^^^^^^^^^^^^^^^^^^^^^
╵
provider/admin/apiconfig/services/_proxies.scss 9:33 @forward
app/assets/stylesheets/provider/_theme.scss 60:1 @forward
app/assets/stylesheets/provider/themes/main.scss 1:1 root stylesheet
| config.assets.enabled = true | ||
|
|
||
| config.assets.precompile = [] | ||
| config.assets.precompile << ->(filename, _path) do |
There was a problem hiding this comment.
Proc in config.assets.precompile is not supported in sprockets v4...
Also, this was moved to config/initializers/assets.rb, this is apparently the new Rails standard.
There was a problem hiding this comment.
Actually, we don't need config.assets.precompile, because our precompile configuration now sits in app/assets/config/manifest.js. We now explicitly define all precompiled assets.
config/initializers/assets.rb
Outdated
|
|
||
| Rails.application.config.assets.paths << Rails.root.join("app/assets/builds") | ||
|
|
||
| Rails.application.config.assets.precompile += %w[ |
There was a problem hiding this comment.
I don't really know what I'm doing...
| @@ -0,0 +1,3 @@ | |||
| Rails.application.config.dartsass.builds = { | |||
There was a problem hiding this comment.
| runs-on: ubuntu-latest | ||
| steps: | ||
| - uses: actions/stale@v9 | ||
| - uses: actions/stale@v10 |
There was a problem hiding this comment.
Hm... not sure why this is here, I don't think I touched it, but fine...
.gitignore
Outdated
| !/assets/jspm_packages/system-polyfills.js | ||
| /public/assets/ | ||
| /lib/developer_portal/app/assets/javascripts/*.map | ||
| /app/assets/builds |
There was a problem hiding this comment.
This is where dartsass puts the compile .css files that we later feed to sprockets:
$ tree app/assets/builds
app/assets/builds
├── error.css
├── provider
│ ├── layouts
│ │ └── iframe.css
│ ├── sites
│ │ └── usage_rules.css
│ └── themes
│ ├── main.css
│ └── wizard.css
├── swagger-ui
│ └── threescale.css
└── vendor
├── c3.css
└── jquery.ui.css
7 directories, 8 files
32f154b to
eead06a
Compare
| group :assets do | ||
| gem 'coffee-rails', '~> 5.0' | ||
| gem 'non-stupid-digest-assets', '~> 1.0' | ||
| gem 'non-digest-assets', '~> 2.4.0' |
There was a problem hiding this comment.
This gem was added a decade ago, mainly because of the need to use the signup_v2.js from outside of Rails (the signup form used to be included somewhere on 3scale.net website as an iframe).
https://github.com/3scale/system/pull/5690
Probably, it would be good to review - maybe we don't even need it anymore.
There was a problem hiding this comment.
Are you sure? this guy here has a different opinion...
There was a problem hiding this comment.
Yes, unfortunately, this gem ended up being our only solution for making the background images work 🙃
| / Order matters! Base must go before any other pack: | ||
| = stylesheet_packs_chunks_tag 'patternfly_base' | ||
| = javascript_packs_with_chunks_tag 'dev_portal_cms_toolbar' | ||
| = stylesheet_link_tag "font-awesome" unless Rails.env.test? |
There was a problem hiding this comment.
We'll just use FontAwesome v5 from Patternfly 4 (included from patternfly_base).
| - if @page.respond_to?(:draft) && draft | ||
| button class="pf-c-button pf-m-plain" type="button" title="Toggle toolbar" id='cms-toolbar-toggle' | ||
| i class="fa fa-caret-square-o-right" aria-hidden="true" | ||
| i class="fa fa-caret-square-right" aria-hidden="true" |
There was a problem hiding this comment.
The new icon is slightly different.
The one that will be exactly the same would be this one https://fontawesome.com/v5/icons/caret-square-right?f=classic&s=regular
but it's far style, and it's not included in Patternfly, see patternfly/patternfly#1457
Not a big deal, IMO though.
There was a problem hiding this comment.
If we followed PF guidelines, we could replace it with an X.
There was a problem hiding this comment.
This is not the "close" button, it's "collapse".
The close button appears on Published tab, and looks like this:
There was a problem hiding this comment.
Yes, but according to PF guidelines, a drawer uses the X. We could use a hamburger also.
mayorova
force-pushed
the
remove-sass-rails
branch
2 times, most recently
from
6e63131 to
a96005e
Compare
mayorova
force-pushed
the
remove-sass-rails
branch
from
934e966 to
5bc2d9f
Compare
| System.redis = System::RedisPool.new(System::Application.config.redis) | ||
| end | ||
|
|
||
| config.assets.quiet = true |
There was a problem hiding this comment.
moved above, to be close to the other config.assets.
mayorova
force-pushed
the
remove-sass-rails
branch
from
5bc2d9f to
1aea980
Compare
|
|
||
| //= link_tree ../javascripts .js | ||
|
|
||
| //= link codemirror/modes/liquid.js |
There was a problem hiding this comment.
I think we need this despite the link_tree ../javascripts .js line, because this file is in vendor/assets/javascripts, and not in app/assets/javascripts.
| xmag_16.png | ||
| dashboard_arrow.gif | ||
| cross.png | ||
| tick.png | ||
| child_metric_background.png | ||
| spinner.svg | ||
| up_green.png | ||
| down_green.png | ||
| clear-left.png | ||
| key.png | ||
| provider/key-background.png | ||
| drag-icon.png |
There was a problem hiding this comment.
This is because image-url helpers don't work with dartsass, and we need to use non-fingerprinted versions, e.g. background-image: url('/assets/provider/key-background.png');
There was a problem hiding this comment.
Are we sure about these .css files? Are they treated as sass files? Maybe they are loaded directly into old dev portals and we break something?
There was a problem hiding this comment.
Yeah, I think it should be OK.
The ones that are being loaded into the CMS are these: https://github.com/3scale/porta/tree/24d8edf20203c76cae5749e925caecc79305ef17/lib/developer_portal/app/views/developer_portal/css (those are the ones the customer can control, and are loaded as CMS templates).
This stats for example is referenced here for example:
(it's a template partial in the CMS). This CSS is served through the assets pipeline:
There was a problem hiding this comment.
While writing this I realized that you are talking about another stylesheet... Honestly, I think this one (as well as selector.css) is broken, but I'll take a closer look.
There was a problem hiding this comment.
Dunno, I think it's broken...
In master, when I build the assets, these ones are built as
/assets/provider/stats/base-95a57af82fdc7d9c2d86d29b83e7c84288c198acce44286452914ecb1c9bfe5b.css
/assets/provider/stats/selector-937604d729193308c9e864fa9cac584d51cb522145940316e94b69b2a08dae61.css
First, I am not sure how I can fetch those from the developer portal...
When I do {{ 'provider/stats/base.css' | stylesheet_link_tag }}, the actual asset that is being fetched is:
http://provider.3scale.localhost:3000/buyer/provider/stats/base.css
(I don't know what adds the /buyer prefix)
If I load the .css directly: http://provider.3scale.localhost:3000/assets/provider/stats/base-95a57af82fdc7d9c2d86d29b83e7c84288c198acce44286452914ecb1c9bfe5b.css
Here is the content I'm getting:
/* Serving legacy dev portals */
@use "_base.css";
/*# sourceMappingURL=data:application/json;charset=utf-8,%7B%22version%22:3,%22sourceRoot%2[...]
This doesn't seem a valid CSS anyway 🙃
So, probably we can just remove these two. But could be in another PR.
Deprecation Warning [slash-div]: Using / for division outside of calc() is deprecated and will be removed in Dart Sass 2.0.0. Recommendation: math.div(5, 4) or calc(5 / 4) More info and automated migrator: https://sass-lang.com/d/slash-div Deprecation Warning [global-builtin]: Global built-in functions are deprecated and will be removed in Dart Sass 3.0.0. Use color.adjust instead. More info and automated migrator: https://sass-lang.com/d/import
mayorova
force-pushed
the
remove-sass-rails
branch
from
1aea980 to
03b39aa
Compare
What this PR does / why we need it:
We are currently using
sass-rails (5.0.8)which depends onsass (3.4.25), known as "Ruby Sass", and it was deprecated in 2018 and stopped maintenance in 2019, see https://sass-lang.com/blog/ruby-sass-is-deprecated/Because of it, we're stuck with sprockets v3, and it's incompatible with Ruby 3.3, to which we want to ugprade in #4129
So, migrating to
dartsass-railsand upgrading to sprockets v4 is a prerequisite for further Ruby upgrades, and in general it's a good thing to get rid of outdated legacy dependencies.0
Which issue(s) this PR fixes
https://issues.redhat.com/browse/THREESCALE-9588
Verification steps
bundle exec rails assets:precompileshould work, and all styles should be present.Special notes for your reviewer:
Previously (with
sass-rails) sprockets (v3) was handling CSS files compilation, delegating translation from.scssto.cssto Ruby Sass.This PR performs the following changes:
Migration from
sass-railstodartsass-railsThis introduces a significant change in the workflow, because now the CSS compliation needs to be performed in 2 steps:
.scssfiles into.css. By default, these assets are compiled into theapp/assets/buildsdirectory. The configuration for dartsass sits inconfig/initializers/dartsass.rb.Good new is that
rake dartsass:buildwhich is the task that builds the assets is integrated withrake assets:precompile- so no need to rundartsass:buildfor production assets,assets:precompileis enough, as before.Bad news is that in development environment, the on-the-fly compilation of assets will not perform
.scssassets compilation, so we need to runrake dartsass:watchif we want to keep the CSS up-to-date with local files changes. This PR integrates this insideyarn devscript (inpackage.json), so in development we can still userake webpack:devas before (but the output is slightly different - we had to sacrifice the progress bar for assets compilation).Sprockets upgrade
This upgrade also required switching from sprockets v3 to v4. See https://github.com/rails/sprockets/blob/main/UPGRADING.md
One of the changes, for example, is that now the assets to be precompiled by Sprockets are configured in
app/assets/config/manifest.jsrather than inconfig.assets.precompile. You can see that we specifically includeapp/assets/buildsas one of the assets paths in the manifest, but NOTE - they files inside it are already.cssfiles, so sprockets does not process them any further - just adds fingerprints etc.Image URL issue
Due to these changes described above, we lose the ability to use
image-urlandasset-urlhelpers that were previously available in Sprockets+Sass, which allowed to replace the simple paths to fingerprinted. Example:would be trasformed to
We can't do it now, because the DartSass runs first and converts
.scssto.css, and sprockets (the one that has knowledge of what fingerprint to append) does not process these files, so can't replace the links.This is the main show-stopper currently.
The current workaround is adding these images to
NonDigestAssetsassets list, which provides not only fingerprinted URLs to the assets, but also without fingerprints, and makes the images available on the server with the plain name (e.g./assets/tree-line.png).Associated changes
Changing to DartSass required quite a lot of changes to get rid of old deprecated syntax, namely:
@importstatements with@useand@forwardand using proper namespacing/) withmath.divopacify,fade-in, etc.) withcolor.adjust()FontAwesome upgrade
Previously FontAwesome v4 was used (through the gem). This gem was also outdated, and not compatible with DartSass.
I decided to instead use the FontAwesome v5, which already forms part of PatterFly v4.
This means that some icons have been updated, others renamed, some are not available etc. See the comparison here: #4145 (comment)
Also, because of the need to import the PF4 styles on the Onboarding Wizard, the style (font primarily) has changed somewhat, which is fine (now it's the same font as the rest of portal).
Other changes
Some other smaller changes have been applied - for example, getting rid of icons in Authentication Providers page. Adhering to the styles configured in Qlty... etc.
Please check out the commits one by one, as this makes the review simpler.