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

React #912

Merged
merged 10 commits into from
Dec 7, 2018
Merged

React #912

merged 10 commits into from
Dec 7, 2018

Conversation

guaka
Copy link
Contributor

@guaka guaka commented Dec 4, 2018

Continuation of #790 after #911

npm run build:prod broke #910

Nice to have merged before this: #913

package.json Outdated
"exports-loader": "^0.7.0",
"expose-loader": "^0.7.5",
"faker": "~4.1.0",
"firebase": "^3.9.0",
Copy link
Contributor

Choose a reason for hiding this comment

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

Let's add this back to master in a separate PR.

#911 (comment)

Copy link
Contributor

@mrkvon mrkvon left a comment

Choose a reason for hiding this comment

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

I'm trying to make this run with start:prod. It's finally going well when removing some pieces here. But I'd like to understand what I'm breaking by removing these pieces.
@nicksellen I'd appreciate a feedback.

r.keys().forEach(path => {
const Component = r(path).default;
const name = extractComponentNameFromPath(path);
if (name !== Component.name) {
Copy link
Contributor

Choose a reason for hiding this comment

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

What is the motivation for this check @nicksellen? The application running via start:prod fails here and the check doesn't seem to be required. Maybe we could disable this in production env?

Copy link
Contributor

Choose a reason for hiding this comment

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

I changed this in 68dece5. To see the original, see the branch at 5efdfa6.

const photo = photos[name];
const file = (window.innerWidth <= 480 && photo.file_mobile) ? photo.file_mobile : photo.file;
const imageUrl = `/img/board/${file}`;
getApplicationScope().$emit('photoCreditsUpdated', { [name]: photo });
Copy link
Contributor

Choose a reason for hiding this comment

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

What is the motivation for having this @nicksellen? It doesn't seem to be required for the application to function and the application fails when it's run with start:prod script.
(I also changed the ng-annotate-loader in config/webpack/webpack.config.js to babel-plugin-angularjs-annotate, so maybe these two fail when together.)

Copy link
Contributor

Choose a reason for hiding this comment

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

I changed this in 68dece5. To see the original, see the branch at 5efdfa6.

@mrkvon mrkvon merged commit c7c1f17 into master Dec 7, 2018
@mrkvon
Copy link
Contributor

mrkvon commented Dec 7, 2018

Merged. Happy React!

@simison
Copy link
Contributor

simison commented Dec 7, 2018

Happy React to everyone! 🎉

Huge kudos to both @mrkvon @nicksellen 👏 Nicely done!

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

Successfully merging this pull request may close these issues.

4 participants