Skip to content
This repository has been archived by the owner on Feb 17, 2021. It is now read-only.

[WIP] Use webpacker for react #204

Open
wants to merge 13 commits into
base: master
Choose a base branch
from
Open

Conversation

csbailey5t
Copy link
Collaborator

Switch to using the webpacker gem to allow us to take advantage of benefits of modern js; refactor react and such to fit the new build process.

@codecov-io
Copy link

codecov-io commented Jul 5, 2017

Codecov Report

Merging #204 into master will decrease coverage by 0.04%.
The diff coverage is n/a.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #204      +/-   ##
==========================================
- Coverage   98.44%   98.39%   -0.05%     
==========================================
  Files          40       40              
  Lines         450      436      -14     
==========================================
- Hits          443      429      -14     
  Misses          7        7
Impacted Files Coverage Δ
app/models/histogram.rb 100% <0%> (ø) ⬆️
app/models/collection.rb 100% <0%> (ø) ⬆️
app/models/image.rb 100% <0%> (ø) ⬆️

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 bc4fd5f...814f1db. Read the comment docs.

@@ -27,5 +29,5 @@ class ImageTemplateCropper extends React.Component {
}

ImageTemplateCropper.propTypes = {
onAddNewTemplate: () => {},
onAddNewTemplate: PropTypes.func,

Choose a reason for hiding this comment

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

propType "onAddNewTemplate" is not required, but has no corresponding defaultProp declaration react/require-default-props

/* global IiifCropper */
import React from 'react';
import PropTypes from 'prop-types';
import IiifCropper from './iiif_cropper';

Choose a reason for hiding this comment

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

Unable to resolve path to module './iiif_cropper' import/no-unresolved
Missing file extension for "./iiif_cropper" import/extensions

@@ -1,6 +1,8 @@
/* global IiifCropper */
import React from 'react';
import PropTypes from 'prop-types';

Choose a reason for hiding this comment

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

Unable to resolve path to module 'prop-types' import/no-unresolved
Missing file extension for "prop-types" import/extensions

@@ -1,6 +1,8 @@
/* global IiifCropper */
import React from 'react';

Choose a reason for hiding this comment

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

Unable to resolve path to module 'react' import/no-unresolved
Missing file extension for "react" import/extensions


describe('LeafletIiif', () => {
it('sets up the iiif layer', () => {
xit('sets up the iiif layer', () => {

Choose a reason for hiding this comment

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

'xit' is not defined no-undef

},

resolveLoader: {
modules: ['node_modules']

Choose a reason for hiding this comment

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

Missing trailing comma comma-dangle

extensions: settings.extensions,
modules: [
resolve(settings.source_path),
'node_modules'

Choose a reason for hiding this comment

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

Missing trailing comma comma-dangle

new ExtractTextPlugin(env.NODE_ENV === 'production' ? '[name]-[hash].css' : '[name].css'),
new ManifestPlugin({
publicPath: output.publicPath,
writeToFileEmit: true

Choose a reason for hiding this comment

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

Missing trailing comma comma-dangle

},

module: {
rules: sync(join(loadersDir, '*.js')).map(loader => require(loader))

Choose a reason for hiding this comment

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

Missing trailing comma comma-dangle

output: {
filename: '[name].js',
path: output.path,
publicPath: output.publicPath

Choose a reason for hiding this comment

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

Missing trailing comma comma-dangle

"dependencies": {},
"dependencies": {
"autoprefixer": "^7.1.1",
"babel-core": "^6.25.0",
Copy link
Collaborator

Choose a reason for hiding this comment

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

Should we consider all of these "dependencies"? Some of them feel like "devDependencies"?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I should be able to move a bunch to devDependencies. Some of these were automatically placed here by the webpacker gem. I can try moving them and see if it breaks things.

Copy link
Collaborator

Choose a reason for hiding this comment

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

ah gotcha.

@@ -0,0 +1 @@
@import '~leaflet/dist/leaflet'
Copy link
Collaborator

Choose a reason for hiding this comment

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

Is it possible to use .scss for consistency?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Should be possible.

@mejackreed
Copy link
Collaborator

When running locally in RAILS_ENV=production after precompiling the assets, I'm not able to get any assets to load in the application. Have you been able to get this going yet?

Also, where are we at with testing our deploy?

@csbailey5t
Copy link
Collaborator Author

@mejackreed I haven't tried it in the production env yet. We also don't have a deploy attempt yet for this. I suspect we'll need to hold on finishing this a bit and turn to other priorities for a while

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants