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

bug 1379288: Include optional locale-specific CSS #4303

Merged
merged 2 commits into from Jul 11, 2017

Conversation

Projects
None yet
3 participants
@jwhitlock
Member

jwhitlock commented Jul 7, 2017

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 Jul 7, 2017

@codecov-io

This comment has been minimized.

Show comment
Hide comment
@codecov-io

codecov-io 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.

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

This comment has been minimized.

Show comment
Hide comment
@stephaniehobson

stephaniehobson Jul 7, 2017

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.

Contributor

stephaniehobson commented Jul 7, 2017

  • 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

This comment has been minimized.

Show comment
Hide comment
@jwhitlock

jwhitlock Jul 7, 2017

Member

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.

Member

jwhitlock commented Jul 7, 2017

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.

@stephaniehobson

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

@stephaniehobson

This comment has been minimized.

Show comment
Hide comment
@stephaniehobson

stephaniehobson Jul 7, 2017

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.

Contributor

stephaniehobson commented Jul 7, 2017

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

This comment has been minimized.

Show comment
Hide comment
@jwhitlock

jwhitlock Jul 7, 2017

Member

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.

Member

jwhitlock commented Jul 7, 2017

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

This comment has been minimized.

Show comment
Hide comment
@jwhitlock

jwhitlock Jul 10, 2017

Member

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

Member

jwhitlock commented Jul 10, 2017

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

@stephaniehobson

This comment has been minimized.

Show comment
Hide comment
@stephaniehobson

stephaniehobson Jul 10, 2017

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.

Contributor

stephaniehobson commented Jul 10, 2017

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.

jwhitlock added some commits Jul 7, 2017

bug 1379288: Include optional locale-specific CSS
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

This comment has been minimized.

Show comment
Hide comment
@jwhitlock

jwhitlock Jul 11, 2017

Member

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

Member

jwhitlock commented Jul 11, 2017

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

@jwhitlock jwhitlock merged commit 9ef0a95 into mozilla:master Jul 11, 2017

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details

@jwhitlock jwhitlock deleted the jwhitlock:locale-specific-css branch Jul 11, 2017

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment