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

bug 1379288: Include optional locale-specific CSS #4303

Merged
merged 2 commits into from Jul 11, 2017

Conversation

@jwhitlock
Copy link
Contributor

@jwhitlock 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
Copy link

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

@stephaniehobson 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
Copy link
Contributor Author

@jwhitlock 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.

Copy link
Contributor

@stephaniehobson stephaniehobson left a comment

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

@stephaniehobson
Copy link
Contributor

@stephaniehobson 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
Copy link
Contributor Author

@jwhitlock 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 jwhitlock force-pushed the jwhitlock:locale-specific-css branch from 4583f91 to 09f3702 Jul 10, 2017
@jwhitlock
Copy link
Contributor Author

@jwhitlock jwhitlock commented Jul 10, 2017

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

@stephaniehobson
Copy link
Contributor

@stephaniehobson 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 2 commits Jul 7, 2017
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 jwhitlock:locale-specific-css branch from 09f3702 to d745c9b Jul 11, 2017
@jwhitlock
Copy link
Contributor Author

@jwhitlock 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 mdn:master Jul 11, 2017
1 check passed
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
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

3 participants