Skip to content
This repository was archived by the owner on Aug 26, 2022. It is now read-only.

bug 1379288: Include optional locale-specific CSS #4303

Merged
merged 2 commits into from
Jul 11, 2017

Conversation

jwhitlock
Copy link
Contributor

Include a locale-specific CSS file if it has been defined in the pipeline. Empty examples for ru and zh-CN are included, as well as helper methods for making it easier to declare these in the settings file.

I install node 8 locally, and I'm unable to get gulp working again. I suspect it should still work with gulp, but I'm not sure. I used make collectstatic inside the container. It may need to be run twice.

@jwhitlock jwhitlock requested a review from stephaniehobson July 7, 2017 22:17
@codecov-io
Copy link

codecov-io commented Jul 7, 2017

Codecov Report

Merging #4303 into master will increase coverage by <.01%.
The diff coverage is 100%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #4303      +/-   ##
==========================================
+ Coverage    88.3%   88.31%   +<.01%     
==========================================
  Files         163      163              
  Lines       10205    10210       +5     
  Branches     1413     1414       +1     
==========================================
+ Hits         9012     9017       +5     
  Misses        967      967              
  Partials      226      226
Impacted Files Coverage Δ
kuma/settings/common.py 94.41% <100%> (+0.13%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 474c282...d745c9b. Read the comment docs.

@stephaniehobson
Copy link
Contributor

  • The ru.css file is being included on the home page.
  • There is no ko.css file and it is correctly not included.
  • gulp is watching the files but not moving them to where pipeline needs them, the gulp styles task needs to be updated like this:
// compiles top-level .scss files
gulp.task('styles', () => {
    // only process files in /styles root
    // all other .scss files are components/includes/libs
    gulp.src('./kuma/static/styles/*.scss')
        .pipe(sass().on('error', sass.logError))
        // send compiled files to where expected by Django Pipeline
        .pipe(gulp.dest('./static/styles'));
    // except locale files, compile and move those too
    gulp.src('./kuma/static/styles/locales/*.scss')
        .pipe(sass().on('error', sass.logError))
        // send compiled files to where expected by Django Pipeline
        .pipe(gulp.dest('./static/styles/locales'));
});

Collect static did not work for me but build-static did.

@jwhitlock
Copy link
Contributor Author

Do you want to add the gulp commit for credit, or do you want me to add it?

I think running make collectstatic twice would have worked as well. make build-static runs make compilejsi18n then make collectstatic. The first call copies the file to static/styles/locales/ru.scss, and the second builds it to static/styles/locales/ru.css. Or something like that. Needs fixing in a future PR.

Copy link
Contributor

@stephaniehobson stephaniehobson left a comment

Choose a reason for hiding this comment

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

Works great with production pipeline, just needs a tweak to work with gulp locally.

@stephaniehobson
Copy link
Contributor

I ran docker-compose exec web ./manage.py collectstatic twice. Is that different from make collectstatic ? Should we be worried running it twice didn't work for me? Will needing to run it twice cause problems when this goes to stage?

I don't need the credit for the gulp change.

@jwhitlock
Copy link
Contributor Author

No, your install is not broken. There's difference when running ./manage.py collectstatic and make collectstatic. It is too dumb to get into.

gulp file updated.

@jwhitlock jwhitlock force-pushed the locale-specific-css branch from 4583f91 to 09f3702 Compare July 10, 2017 15:04
@jwhitlock
Copy link
Contributor Author

Rebased, removed unused future code setting variant, to make code coverage happier.

@stephaniehobson
Copy link
Contributor

Gulp is handling changes now :)

I think this is good to go but we could also remove one of the two files since they are empty.

Include a locale-specific CSS file if it has been defined in the
pipeline. Empty example for zh-CN is included, as well as helper methods
for making it easier to declare these in the settings file.
@jwhitlock jwhitlock force-pushed the locale-specific-css branch from 09f3702 to d745c9b Compare July 11, 2017 14:59
@jwhitlock
Copy link
Contributor Author

I dropped the Russian one, because the zh-CN.scss shows how to handle locales with hyphens.

@jwhitlock jwhitlock merged commit 9ef0a95 into mdn:master Jul 11, 2017
@jwhitlock jwhitlock deleted the locale-specific-css branch July 11, 2017 15:15
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
3 participants